Fix spring sub-frame sampling, MotionValue reactive-read refcount, and PresenceChild map - #81
Fix spring sub-frame sampling, MotionValue reactive-read refcount, and PresenceChild map#81JonathonRP wants to merge 2 commits into
Conversation
…d PresenceChild map
- useSpring: restore upstream's animation.sample(frameData.delta) when retargeting a spring whose previous animation has not yet rendered a frame.
- MotionValue: track Svelte's signal subscriber separately from user on('change') subscribers so reactive .current reads no longer participate in the refcount that auto-stops animations.
- PresenceChild: drop the no-op \ wrapper around the registered-children Map.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: d13d8ea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
commit: |
Only create the SubscriptionManager backing Svelte's signal subscriber on the first reactive read, mirroring how events managers are created on first on(). Most MotionValues are never read inside a \/\, so this avoids ~4 allocations per value. Also drops a stale commented-out #subscribe declaration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Three small, independent correctness fixes against framer-motion v11.11.11 behaviour.
Note
File overlap with #80. PR #80 (
jonathonrp-backport-upstream-bug-fixes) also editspackages/motion-start/src/value/index.ts, wideningOwner['getProps'](line 50) to declaretransformTemplate. That is written as a method shorthand on purpose — it relies on parameter bivariance for theAcceleratedAnimation.supports()transformTemplate guard, so it must not be "tidied" into a property signature.This PR's hunks in that file are confined to the
MotionValueclass body (the#reactiveSubscribers/#subscribefields, a stale commented-out line, and the notify branch ofupdateAndNotify);Owner/getPropsis untouched, nothing was reformatted or reorganised, and the change to howchangesubscribers are stored is additive with no bearing on theOwnertyping. The two diffs do not overlap and should merge cleanly in either order. Likely order is #80 first, then this rebased on top.1.
value/use-spring.ts— restore dropped sub-frame samplingUpstream calls
animation.sample(frameData.delta)when retargeting a spring whose previous animation has not yet rendered a frame, so the replacement spring starts from the correct interpolated position. The port dropped that call, so a spring retargeted before its first render restarted from the stale position (a visible "snap back" on rapid retargets).Restored the upstream branch, adapted to this file's structure (
activeSpringAnimationis a plain closure variable rather than a React ref), and addedframeDatato the existing frameloop import.Test:
src/value/__tests__/use-spring.svelte.spec.tsmounts a spring, sets a target, and retargets it before the first animation has ticked. It drives the spring with an idledriver(never ticks) so the preconditionanimation.time === 0is deterministic rather than dependent on frameloop queue ordering, spies onanimateValueto capture the keyframes each animation is started with, and asserts the retargeted animation starts from the sampled position rather than0. Verified failing before the fix (expected 0 to be greater than 0).2.
value/index.ts— reactive.currentreads no longer cancel animations.currentwas made reactive withcreateSubscriber((update) => this.on('change', update)), which registers a genuinechangelistener.MotionValue.on('change', …)'s unsubscribe schedules aframe.readthat callsthis.stop()whenevents.change.getSize()hits zero. So for a standaloneMotionValue(not owned by aVisualElement), unmounting the last component that merely read.currentreactively cancelled an in-flight animation. Upstream has no such coupling — there, subscriber count and animation lifetime are independent in this case.Fix: Svelte's signal subscriber is now held in its own
SubscriptionManager(#reactiveSubscribers), notified fromupdateAndNotifyunder the same_current !== prevcondition as thechangeevent, immediately afterevents.change.notify(...). It is therefore invisible to the refcount that gates auto-stop. Reactive behaviour of.currentis unchanged, and the existing "when all change listeners removed, stop animation" behaviour for realon('change')subscribers still holds.Lazy allocation
#reactiveSubscribersis declared as an optional field and constructed on first reactive subscription (inside thecreateSubscriberstart callback), with#reactiveSubscribers?.notify()at the notify site.This matters because
SubscriptionManageris not free: constructing one allocates the instance, its backingsubscriptions: Handler[] = []array, and two bound functions —addandnotifyare instance fields, not prototype methods — so roughly four allocations each time. MotionValues are created per animated property per element, so on a list of a few hundred animated items an eager class-field initializer would be a meaningful and permanent cost, and the overwhelming majority of MotionValues are never read inside a$derived/$effect, leaving the manager empty for its entire life. Lazy allocation keeps the port aligned with upstream's practice of lazily creatingevents.changeon firston()rather than allocating event managers up front.createSubscriberitself is still constructed eagerly per instance, unchanged from before this PR — for the record it only allocates a closure, asource(0)signal and the returned function, and registers no listeners or effects until the first tracked read.Also removed a stale
// #subscribe: ReturnType<typeof createSubscriber> | null = null;left over from an earlier approach, which was misleading next to the real#subscribedefined above it.Tests in
src/value/__tests__/motion-value-reactivity.svelte.spec.ts:MotionValue, mount and unmount a component that reads.current, assert the animation is still running after the frameloop has processed the teardown. Verified failing before the fix (expected false to be true).SubscriptionManagerconstruction is counted via avi.mockProxyconstructtrap (the field itself is a real#privateand can't be read from a test, so this avoids weakening the source to make it observable). AMotionValuethat is onlyset()constructs zero managers; mounting a component that reads.currentconstructs exactly one. Verified failing with an eager field initializer (expected 1 to be +0).3.
components/AnimatePresence/PresenceChild/PresenceChild.svelte— remove no-op$state(new Map())Chose: drop the
$statewrapper (notSvelteMap).presenceChildrenis only ever read imperatively:handleExitCompleteandregisterare context callbacks invoked by descendants, and the two$effects that touch it are driven byisPresent— theforEach/sizereads happen inside an effect body already trackingisPresent, and thesizeread is inside atick().then()callback where nothing is tracked anyway. Nothing renders from the map or derives from its contents, so it was never meant to drive reactivity, and$statearound a nativeMapis a no-op in Svelte 5 regardless (only plain objects and arrays are proxied). Switching toSvelteMapwould have added reactive overhead for no consumer. Added a comment recording the reasoning. The map is local to the component and never passed out, so nothing depended on the wrapper.Verification
vitest --run src/value: 74 passed (11 files), includingmotion-value-reactivity.svelte.spec.tsanduse-spring.svelte.spec.ts.vitest --runoversrc/value,src/components/AnimatePresenceandsrc/render: 200 passed.package-imports.spec.ts,Reorder production SSR) are known pre-existing flakes that pass in isolation, including on a clean stashed tree.bun run --filter motion-start check(svelte-check): 0 errors, 0 warnings.biome checkis already failing on pre-existing formatting/line-ending issues; baseline compared file-by-file)..changeset/tidy-spiders-jam.md(patch).