Conversation
…c branches A child created inside a dynamic branch is disposed by that branch's queued update job, which runs after synchronous consumers of its props have been notified. The dirty check of a `flush: 'sync'` watcher therefore refreshed the prop source computed while the branch was still alive, re-running `() => x.y` after the guard expression that produced it had already become invalid. Props of a child created inside a dynamic branch now commit through a detached scope that the branch teardown stops, the same mechanism KeepAlive uses for cached components: reads stay on the last committed value, the branch disposal removes the commit effect's dependencies before the invalidating mutation can notify it, and no read happens after disposal. fixes vuejs#15673 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ee9c5ce-96b9-4d2b-a163-a6e5a4a4eb38
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Overlap disclosure: @edison1105's #15696 and #15708 touch the same This PR's scope is only the read side — a prop read inside a If either of yours covers that case, close this one and I will not spend your review time on it. |
|
Thanks for the PR, closing in favor of #15708 |
2 participants
Fixes #15673. The reproduction now behaves like VDOM and like 3.5: the branch switches to
x is goneand nothing is thrown.Mechanism (measured, not inferred)
With the two tests below applied to the pristine base (
08cad7648), the first one fails with:x.value = undefinedtriggers the ref, which notifies the child'sflush: 'sync'watcher inline. Its dirty check refreshes the prop source cache (resolveFunctionSourcewraps the compiled() => x.yin acomputed), sox.yis evaluated after thev-ifguard expression became invalid but before the guard's queued job disposed the branch that owns the child. The tail of the stack (trigger→endBatch) shows the error leaves the ref assignment itself: there is nocallWithErrorHandling/handleErrorframe, and it is not routed toapp.config.errorHandler. Render-only reads and pre/post watchers run later in the same flush, after the branch teardown, which is why only the synchronous-observation path can observe the stale source.Fix
packages/runtime-vapor/src/component.ts— a child created inside a dynamic branch now commits its prop sources through a detachedEffectScopethat the branch teardown stops, reusing the existingisolatePropSourcesmechanism (packages/runtime-vapor/src/componentProps.ts) that KeepAlive already applies to cached components. The isolated props read ashallowReactivecommitted target written by an immediaterenderEffect:@vue/reactivity, measured with a throwaway spec)const c = computed(() => x.value.y); c.value;thenx.value = undefined, no watcherwatch(() => x.value.y, fn, { flush: 'sync' })in a scope,scope.stop(), thenx.value = undefinedwatch(() => x.value.y, fn, { flush: 'sync' }), thenx.value = undefinedCannot read properties of undefined (reading 'y')watch(computed(() => x.value.y), fn, { flush: 'sync' })(the vapor prop shape), thenx.value = undefinedAlso in this commit:
packages/runtime-vapor/src/fragment.tsmarks the branch currently rendering (currentBranchFragment) aroundDynamicFragment.renderNodes(), saved/restored so nested branches and deferred (transition/KeepAlive) renders are correct.unmountComponentand the VDOM-interop unmount stopinputScopeunconditionally, instead of only whenisKeepAliveEnabled. The scope previously existed only on the KeepAlive path, so the gate was a no-op there; without this the new scope would never be stopped in apps that do not useKeepAlive.v-oncechildren are skipped (once), matching the existing KeepAlive path, because their props are snapshotted at creation.Negative control on the unmodified revision
git stash push -- packages/runtime-vapor/src, then the same tree with the same tests:Full suite, same revision (
08cad7648, sources pristine, tests added):Full suite with the fix:
The only movement is the two new tests, failed → passed; nothing else changed on this base.
Tests added
packages/runtime-vapor/__tests__/componentProps.spec.ts(parity harness, so each case is asserted against VDOM in the same run):sync watcher does not re-read a source disposed by a v-if branch— the issue's shape; assertsseen.vdom === [],seen.vapor === seen.vdom,vapor.text === 'x is gone' === vdom.text.sync watcher of a v-if child still sees committed prop updates— the fix must not silence legitimate updates: asserts the watcher sees['b']in both modes after the flush, and that neither mode observes the value before the flush that commits it.Timing: an existing divergence closed, not introduced
A throwaway probe (not committed) recorded a
flush: 'sync'watcher on av-ifchild whilex.ychanged:['b']['b'][]['b'][]['b'][]['b']So before this change Vapor delivered a synchronous prop read that VDOM never delivers — a divergence on its own. The fix makes Vapor match VDOM by publishing props at commit time, which is the semantics the KeepAlive path (#15228) already established. The second added test pins it, so this is a deliberate, asserted behavior change and not an accident: a
flush: 'sync'consumer of a prop inside a dynamic branch now observes updates during the flush rather than inline with the mutation. Pre/post watchers and render reads are unaffected (they always ran in the flush).Answers to the issue's claims
() => x.y" — confirmed; the throwing frame is the compiled SFC getter (_utils.ts:221), reached from the prop-source cache atcomponentProps.ts:211.resolveFunctionSourcecaches it in acomputedowned by the parent" — half right, and the distinction does not change the outcome.resolveFunctionSourceevaluates the source in the parent's context but collects the cachecomputedin the active consumer scope (getCurrentScope()), i.e. the child's watcher/render scope, withonScopeDispose(() => (source._cache = undefined)). It is acomputedwith the child as its active consumer, which is why the child's sync watcher's dirty check is what refreshes it (stack frames 4–6).xnotifies the child's sync watcher, whose dirty check refreshes that computed (runningx.y) before thev-ifbranch has been disposed" — confirmed exactly; the guard's job is queued in the same flush and runs after the inline sync notification.trigger→endBatch), there is no error-handling frame,app.config.errorHandlernever sees it, and the test fails on thedata.value.x = undefinedline.Known limitations / unverified
DynamicFragmentrender (sov-if/v-else, dynamic components and slot-outlet fragments), not onlycreateIf. Children insidev-for/ForBlockare deliberately not marked: I probed the analogous case on pristine and no stale read happens there, because av-foritem's prop source depends on the item, not on the collection that was invalidated (probe result on both revisions:{ vdom: [], vapor: [] }, no throw), so there is nothing to fix.rawPropsis isolated for dynamic branches;rawSlots.$keeps its closure semantics for that path. A slot whose closure invalidates a prop source after disposal was not reproduced and is not covered here.v-if) is out of scope: nothing is disposed there, the read is legitimate and the throw is inherent to the user's code — plain reactivity throws identically (table above), which is the shape reported in Reactivity (3.6): a throw while notifying one subscriber leaves the rest of the batch unnotified until the next unrelated write #15674; this PR does not touchpackages/reactivity.v-ifdynamic branches is only covered by the existing suite (green), not by a dedicated new test../node_modules/.bin/vp lintclean,./node_modules/.bin/vp fmt --checkclean,npx --yes pnpm@12.4.2 tsc --incremental --noEmitwith no diagnostics (the pre-commit hook ran lint andtscagain on the staged diff and passed).packages/reactivitywas not modified, so its suite was not run.