fix(vue-form): run the cleanup returned by FormApi.mount() on unmount - #2364
fix(vue-form): run the cleanup returned by FormApi.mount() on unmount#2364ktx-kirtan wants to merge 2 commits into
Conversation
`onMounted(formApi.mount)` discards the teardown function that `FormApi.mount()` returns. Unlike React's `useEffect`, Vue ignores a value returned from `onMounted`, so the three `formEventClient` listeners (`request-form-state`, `request-form-reset`, `request-form-force-submit`) and the devtools store subscription registered by `mount()` are never released. Every mounted form therefore leaks them for the lifetime of the document, and because the listener closures capture the `FormApi` — and through it the options object supplied by the component — the component's scope is retained too. In a SPA that mounts a form on several routes this accumulates on every navigation. `react-form` already gets this right via `useIsomorphicLayoutEffect(formApi.mount, [])`, which does honour the returned cleanup; this brings the Vue adapter in line.
Covers the regression directly: every window listener the devtools event client registers during FormApi.mount() must be removed when the component unmounts, so repeatedly mounting a form does not accumulate listeners.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesVue form lifecycle cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change ensures Vue forms release their event listeners and subscriptions when unmounted, preventing cleanup-related memory leaks without altering normal form behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and directly addresses the problem, fix, motivation, test coverage, and observed impact. It does not use the template's exact Changes, Checklist, or Release Impact headings, and it does not explicitly confirm the checklist items or changeset status, but the required change information is otherwise substantially complete.
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
patch-package binds a patch to one exact version, so upgrading a patched package silently stops the patch matching. On the default settings that is only a warning and the install still exits 0 — the memory-leak fixes in web/patches would come back off without anyone noticing. --error-on-warn makes any drift a non-zero exit, including the case where the patch still applies cleanly to a newer version. Verified by renaming a patch to a version that does not match: the install now exits 1 instead of 0. The packages stay on caret ranges and upgrade normally. When one moves, the install fails in the PR's CI checks naming the package, and the patch is regenerated or dropped from there. Note the npm-update workflow itself only runs `npm install --package-lock-only`, which skips scripts, so the red check lands on the PR it opens rather than in the workflow log. web/patches/README.md documents why each patch exists, what to do when one stops applying — for @tanstack/vue-form the right answer may be deleting it, since the fix is upstream in TanStack/form#2364 — and how to confirm a regenerated patch still fixes the leak rather than merely applying.
Problem
packages/vue-form/src/useForm.tsxdoes:FormApi.mount()returns a teardown function. Unlike React'suseEffect, Vue ignores a value returned fromonMounted, so that teardown is never invoked.mount()registers three listeners on the sharedformEventClientplus a store subscription:Because
formEventClientis a module-level singleton, those listeners live for the lifetime of the document. Every mounted form leaks threewindowlisteners and a store subscription, and the listener closures capture theFormApi— and throughthis.options, the options object supplied by the component — so the component's scope is retained too.form-unmountedis also never emitted, so devtools never learns the form went away.In an SPA that mounts a form on more than one route, this accumulates on every navigation.
Fix
Hold the teardown and call it from
onUnmounted:react-formalready gets this right —useIsomorphicLayoutEffect(formApi.mount, [])does honour the returned cleanup. This brings the Vue adapter in line with the same contract.Tests
Added
packages/vue-form/tests/useFormMountCleanup.test.tsxwith two cases:windowlistener type registered during mount is removed on unmountBoth fail on
mainand pass with this change.How this was found
Profiling a Vue 3 SPA for a memory leak across route changes. Instrumenting
addEventListener/removeEventListenerand capturing construction stacks showed threewindow:form-devtools:*listeners accumulating per navigation, originating inFormEventClient.on. On the affected route this retained ~4,000 detached DOM nodes per navigation; applying this fix locally took that to 0.Note the same discarded-cleanup pattern cannot be worked around by consumers:
useFormcapturesformApi.mountby value when registering the hook, so wrappingmounton the returned instance afterwards has no effect. It has to be fixed here.Summary by CodeRabbit
Bug Fixes
Tests