chore(types): improve type safety in watch functions and instanceWatch #13918
Conversation
WalkthroughTightens TypeScript typings in watcher-related code: narrows dev-only option casts, changes createPathGetter to accept a ComponentPublicInstance and return a union type, and makes dynamic property/path access use keyof and non-null assertions for safer typing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-core/src/apiWatch.ts (1)
266-275: FixcreatePathGettertyping: return a WatchSource (function), not the source union.
createPathGetterreturns a getter function. The current signature/inner return type suggests it returns the “source union” or aWatchEffect, which is inaccurate and harms type inference. Return aWatchSource<unknown>(i.e.,() => unknown) and keep dynamic indexing simple.-export function createPathGetter(ctx: ComponentPublicInstance , path: string): WatchSource | WatchSource[] | WatchEffect | object { - const segments = path.split('.') - return (): WatchSource | WatchSource[] | WatchEffect | object => { - let cur = ctx - for (let i = 0; i < segments.length && cur; i++) { - cur = cur[segments[i] as keyof typeof cur] - } - return cur - } -} +export function createPathGetter( + ctx: ComponentPublicInstance, + path: string, +): WatchSource<unknown> { + const segments = path.split('.') + return (): unknown => { + let cur: any = ctx + for (let i = 0; i < segments.length && cur; i++) { + cur = cur[segments[i]] + } + return cur + } +}
🧹 Nitpick comments (2)
packages/runtime-core/src/apiWatch.ts (2)
24-24: Use type-only, internal import to avoid runtime cycles.Import
ComponentPublicInstanceas a type and from the local module to prevent accidental runtime imports and potential circular deps.-import { ComponentPublicInstance } from '@vue/runtime-core' +import type { ComponentPublicInstance } from './componentPublicInstance'
247-253: Avoid scattered non-null assertions; simplify dynamic key access.Bind
proxyonce with a single non-null assertion and simplify the string-key access. This reduces noise and keeps intent clear.- const publicThis = this.proxy + const publicThis = this.proxy! const getter = isString(source) ? source.includes('.') - ? createPathGetter(publicThis!, source) - : () => publicThis![source as keyof typeof publicThis] + ? createPathGetter(publicThis, source) + : () => (publicThis as any)[source as string] : source.bind(publicThis, publicThis)Please confirm there’s no path where
this.proxycan be null when$watchis invoked (e.g., very early lifecycle or after unmount). If there is, we should guard and return a stopped handle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/src/apiWatch.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/src/apiWatch.ts (4)
packages/shared/src/general.ts (2)
extend(24-24)isString(51-51)packages/runtime-core/src/index.ts (4)
WatchEffectOptions(232-232)ComponentPublicInstance(299-299)WatchSource(234-234)WatchEffect(230-230)packages/runtime-core/src/componentPublicInstance.ts (1)
ComponentPublicInstance(292-345)packages/reactivity/src/watch.ts (2)
WatchSource(39-39)WatchEffect(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (2)
packages/runtime-core/src/apiWatch.ts (2)
70-72: LGTM: dev-only merge for post-flush.Merging
DebuggerOptionsonly in dev while forcing{ flush: 'post' }in prod is correct since debug hooks are no-ops in prod.
81-83: LGTM: dev-only merge for sync-flush.Same rationale as above; looks good.
edison1105
changed the title
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
Thanks for the PR. |
edison1105
added
🧹 p1-chore
…Darkstone/vue-core into feat/watch-source-enhance
…tter and createWatcher
sry, there was a return type error, fixed it ! |
edison1105
added
ready to merge
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/runtime-core/src/apiWatch.ts (2)
251-256: Tighten non-null handling and remove repeated assertions.
Initialize a narrowed variable once and (optionally) guard in dev to avoid surprising NPEs if called too early.Apply this diff:
- const publicThis = this.proxy + const publicThis = this.proxy! + if (__DEV__ && !this.proxy) { + warn(`instanceWatch called on an instance without proxy`) + } const getter = isString(source) ? source.includes('.') - ? createPathGetter(publicThis!, source) - : () => publicThis![source as keyof typeof publicThis] + ? createPathGetter(publicThis, source) + : () => publicThis[source as keyof typeof publicThis] : source.bind(publicThis, publicThis)
270-282: Return type of the getter should be the observed value (unknown), not “watchables”.
createPathGetterreturns a value read from the instance (e.g., number/string/object), not a WatchSource/WatchEffect/array. Using that union conflates “source shape” with “returned value” and can leak into inference. Prefer unknown for the getter’s return type. Also, usingkeyof typeof curhere is brittle; indexing via a structural type is simpler and clearer.Apply this diff:
-export function createPathGetter( - ctx: ComponentPublicInstance, - path: string, -): () => WatchSource | WatchSource[] | WatchEffect | object { +export function createPathGetter( + ctx: ComponentPublicInstance, + path: string, +): () => unknown { const segments = path.split('.') - return (): WatchSource | WatchSource[] | WatchEffect | object => { + return (): unknown => { let cur = ctx for (let i = 0; i < segments.length && cur; i++) { - cur = cur[segments[i] as keyof typeof cur] + cur = (cur as Record<string, unknown>)[segments[i] as string] } return cur } }Please run the repo typecheck and a local ecosystem CI slice to ensure no inference regressions around options-based watchers that rely on
createPathGetter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/src/apiWatch.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-core/src/apiWatch.ts (3)
packages/shared/src/general.ts (2)
extend(24-24)isString(51-51)packages/runtime-core/src/componentPublicInstance.ts (1)
ComponentPublicInstance(292-345)packages/reactivity/src/watch.ts (2)
WatchSource(39-39)WatchEffect(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (3)
packages/runtime-core/src/apiWatch.ts (3)
24-24: Good: importing the concrete public instance type enhances safety.
Switching to a precise type for ctx is appropriate for the later path getter typing.
70-73: Dev-only merge is fine; confirm dropping DebuggerOptions in prod is intentional.
In prod, any DebuggerOptions passed are ignored. That aligns with dev-only behavior of onTrack/onTrigger, but please confirm this is desired.
83-86: Same note for sync variant.
Prod build ignores DebuggerOptions; ensure this matches expectations.
4 participants
removed some any types and used the correct type to ensure type safety
by the way, Is this
WatchSourcenot in line with the expected design? Thewatchapi is not directly using thetype WatchSourceSummary by CodeRabbit
Note: No runtime behavior changes; impact is limited to TypeScript/development-time ergonomics.