fix(adapters): use latest onUnmount callback during cleanup#194
fix(adapters): use latest onUnmount callback during cleanup#194gwagjiug wants to merge 3 commits intoTanStack:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughA patch release updates React and Preact Pacer packages to fix unmount callback handling. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🎯 Changes
This PR fixes a stale cleanup callback issue in adapter hooks that support
onUnmount.Before this change, these hooks registered an unmount cleanup with an empty dependency array and read
mergedOptions.onUnmountdirectly inside that cleanup. That meant the cleanup closed over the value from the first render. If a consumer re-rendered with a newonUnmountcallback, the hook could still call the old callback during unmount.This change keeps the existing "register cleanup once" behavior, but makes the cleanup read the latest
onUnmountcallback instead of the initial one.Updated hooks:
packages/react-pacer/src/debouncer/useDebouncer.tspackages/react-pacer/src/async-debouncer/useAsyncDebouncer.tspackages/react-pacer/src/rate-limiter/useRateLimiter.tspackages/preact-pacer/src/debouncer/useDebouncer.tsWhy this approach
I wanted to keep this fix as small and low-risk as possible.
I considered whether the previous behavior might have been intentional, since the cleanup effect is deliberately registered with
[]to keep teardown stable. However, the rest of these hooks already update runtime behavior from the latest render by reassigningfnand callingsetOptions(mergedOptions)on each render. Given that, treatingonUnmountas "initial render only" felt inconsistent with the rest of the hook contract and with how consumers would naturally expect a cleanup option to behave.Because of that, I treated this as a stale-closure bug rather than an intentional "initial-only" API.
What changed in the implementation
The change is intentionally minimal:
useLatestonUnmountcallback in a refref.currentinside the unmount cleanupSo the behavior changes only in one place:
onUnmountonUnmountMotivation
This matters when
onUnmountdepends on props or render-time values and changes over the component lifecycle. In that case, the old behavior could execute outdated cleanup logic during unmount.This PR fixes that without changing how often the effect is registered or how the default teardown works.
Local validation
pnpm run test:pr --base=53fb52ec --head=HEADSuccessfully ran targets test:eslint, test:sherif, test:knip, test:docs, test:lib, test:types, build for 86 projects and 4 tasks they depend onpnpm run test:ciSuccessfully ran targets test:eslint, test:sherif, test:knip, test:docs, test:lib, test:types, build for 169 projects (3m)pnpm changeset status --since=53fb52ec --verbose@tanstack/react-pacerand@tanstack/preact-pacer✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Release Notes