From 7205a1d668bf77c43ee807d287c6e847bc3e467b Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Fri, 21 Oct 2022 11:14:37 +0100 Subject: [PATCH 1/2] add invalidate old callbacks rfc --- text/0230-invalidating-use-callback.md | 85 ++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 text/0230-invalidating-use-callback.md diff --git a/text/0230-invalidating-use-callback.md b/text/0230-invalidating-use-callback.md new file mode 100644 index 00000000..be5f122a --- /dev/null +++ b/text/0230-invalidating-use-callback.md @@ -0,0 +1,85 @@ +- Start Date: 2022-10-21 +- RFC PR: (leave this empty) +- React Issue: (leave this empty) + +# Summary + +This RFC addresses how the `useCallback` hook is used and how it helps in +inconsistency and encourages memory leaks. + +`useCallback` returns a function reference governed by the attached dependencies, + this function stays alive even if dependencies change. + +`useCallback`'s function often calls a state setter and close over some other +variables, then the returned function is usually sent as a callback in an +asynchronous flow. + +Due to the unpredictable nature of async js (we don't know for sure when it will +call our function), the callback may be called after dependencies change and +obtaining a new function reference, which will lead the component to an +incorrect state. Most of "wrong shown data" errors I debugged had this exact +issue. + +# Solution + +The proposed solution will be just invalidating the previous function whenever +we change the dependencies and give a new reference. While adding a warning +in development mode to the developer may just unsubscribe to abort its async flow. + +One approach I can think of to implement this is something like this: + +```flow js + +function updateCallback(callback: T, deps: Array | void | null): T { + const hook = updateWorkInProgressHook(); + const nextDeps = deps === undefined ? null : deps; + const prevState = hook.memoizedState; + if (prevState !== null) { + if (nextDeps !== null) { + const prevDeps: Array | null = prevState[1]; + if (areHookInputsEqual(nextDeps, prevDeps)) { + return prevState[0]; + } + } + } + + const wrappedCallback = realCallback.bind(null, hook, callback, nextDeps); + hook.memoizedState = [wrappedCallback, nextDeps]; + + return wrappedCallback; +} + +function realCallback( + hook: Hook, + callback: T, + currentDeps: void | null | undefined | any[] +) { + if (areHookInputsEqual(currentDeps, hook.memoizedState[1])) { + return callback.apply(null, arguments); + } else { + if (__DEV__) { + warnAboutStaleUseCallbackCall(); + } + return undefined; + } +} + +function warnAboutStaleUseCallbackCall() { + console.error("A stale useCallback's function was called.") +} + +``` + +The drawbacks for this solution is that it does a full traversal of the +dependencies everytime it is invoked, it can be optimized by altering a boolean +variable in the scope of `updateCallback` and will also require creating* +the real callback there. + +# Final words + +I understand that this _may_ break some already broken software, but I cannot +think of a use case which this invalidation is considered harmful. + +I searched over the old issues but did not find any discussion about this. + +Best regards. From 1b1419c5755effde5b8e3e15192ef8f75a3fc562 Mon Sep 17 00:00:00 2001 From: Mohamed EL AYADI Date: Fri, 21 Oct 2022 11:21:43 +0100 Subject: [PATCH 2/2] RFC: invalidate old useCallback functions --- text/0230-invalidating-use-callback.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/text/0230-invalidating-use-callback.md b/text/0230-invalidating-use-callback.md index be5f122a..6b86a81b 100644 --- a/text/0230-invalidating-use-callback.md +++ b/text/0230-invalidating-use-callback.md @@ -28,7 +28,7 @@ in development mode to the developer may just unsubscribe to abort its async flo One approach I can think of to implement this is something like this: -```flow js +```typescript function updateCallback(callback: T, deps: Array | void | null): T { const hook = updateWorkInProgressHook();