Add ref cleanup function #25686

sammy-SC · 2022-11-15T17:22:19Z

Add option for ref function to return a clean up function.

<div ref={(_ref) => {
  // Use `_ref`
  return () => {
    // Clean up _ref
  };
}} />

If clean up function is not provided. Ref function is called with null like it has been before.

<div ref={(_ref) => {
  if (_ref) {
    // Use _ref
  } else {
    // Clean up _ref
  }
}} />

sizebot · 2022-11-15T18:49:04Z

Comparing: e1dd0a2...ebb8afc

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	+0.13%	153.64 kB	153.83 kB	+0.10%	48.90 kB	48.95 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.13%	155.56 kB	155.75 kB	+0.13%	49.51 kB	49.57 kB
facebook-www/ReactDOM-prod.classic.js	+0.09%	530.41 kB	530.88 kB	+0.11%	94.67 kB	94.77 kB
facebook-www/ReactDOM-prod.modern.js	+0.09%	515.66 kB	516.14 kB	+0.10%	92.49 kB	92.59 kB
facebook-www/ReactDOMForked-prod.classic.js	+0.09%	530.41 kB	530.88 kB	+0.11%	94.67 kB	94.77 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-react-native/react-test-renderer/cjs/ReactTestRenderer-profiling.js	+0.23%	302.54 kB	303.23 kB	+0.18%	53.29 kB	53.38 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js	+0.22%	114.33 kB	114.58 kB	+0.19%	34.35 kB	34.41 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js	+0.22%	114.35 kB	114.60 kB	+0.19%	34.37 kB	34.43 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.min.js	+0.22%	115.77 kB	116.02 kB	+0.22%	34.76 kB	34.84 kB
oss-stable-semver/react-art/cjs/react-art.production.min.js	+0.21%	91.64 kB	91.84 kB	+0.18%	28.24 kB	28.29 kB
oss-stable/react-art/cjs/react-art.production.min.js	+0.21%	91.67 kB	91.86 kB	+0.19%	28.24 kB	28.29 kB
oss-experimental/react-art/cjs/react-art.production.min.js	+0.21%	93.09 kB	93.28 kB	+0.19%	28.65 kB	28.70 kB
react-native/implementations/ReactFabric-profiling.js	+0.21%	333.44 kB	334.13 kB	+0.19%	59.03 kB	59.15 kB
react-native/implementations/ReactNativeRenderer-profiling.js	+0.20%	339.67 kB	340.36 kB	+0.21%	60.01 kB	60.14 kB

Generated by 🚫 dangerJS against ebb8afc

sebmarkbage · 2022-11-16T18:47:41Z

packages/react-reconciler/src/ReactFiberCommitWork.old.js

+        }
+      }
+    }
+  } else if (ref !== null) {
    if (typeof ref === 'function') {


You can place the if (refCleanup !== null) { inside here since you'll always do that check anyway so there's no extra checks. That way when it's not a function but a ref.current you don't do extra checks.

sebmarkbage · 2022-11-16T18:52:11Z

packages/react-reconciler/src/ReactFiberCommitWork.new.js

-          );
-        }
+
+      if (typeof retVal === 'function') {


Normally it's good to check the type as close to where you get it from the user as possible but it'd probably be ok to do this type check during the cleanup instead.

That way there's no extra work on initial mount.

The VM has to check the type anyway before calling it upon cleanup.

sebmarkbage

It's unfortunate that this needs an extra field on every fiber. That's a lot of memory. It might be worth it though. Not sure. Compared to keeping an inner tuple that contains the original ref for comparison and a clean up.

sebmarkbage

Nits.

gaearon · 2022-11-16T21:33:18Z

What does this mean for various utilities that try to "merge" refs? I know we don't encourage them, but is there a reasonable upgrade path for them that works regardless of the ref type? i.e., are they fixable?

sebmarkbage · 2022-11-16T22:29:59Z

They'll have to update to support the new ref type. The raw imperative code is the same capability so there's no reason they couldn't.

KurtGokhan · 2022-11-16T23:01:04Z

@gaearon imho this changes makes merging refs simpler/cleaner. Here is a quick one I came up with next to the old implementation.

https://gist.github.com/KurtGokhan/9aafd8e83c9bc6a2946fe2dc7f2c1d19#file-use-combined-refs-new-ts

Note how the old implementation has to store the refs in a ref but new implementation only stores them locally.

gaearon · 2022-11-17T11:39:54Z

Here is a quick one I came up with next to the old implementation.

The trickier part is supporting all forms at the same time (object, function with null, and new style) cause refs themselves are coming from the user and could be of either kind.

KurtGokhan · 2022-11-17T14:32:44Z

The trickier part is supporting all forms at the same time (object, function with null, and new style) cause refs themselves are coming from the user and could be of either kind.

Yes, my example should cover all cases. When the ref doesn't return a cleanup function, setting the previous ref to null should be same as the old behavior.

gaearon · 2022-11-17T15:53:45Z

Ah i see. You’re using the new style for the combined one which simplifies things. I haven’t thought of that.

dante01yoon · 2022-11-30T02:19:59Z

@sammy-SC
Is there any usage examples using this ref clean up function? do you expect this changes gives brevity in codes?

Summary: Three problems popped up during the sync: - facebook/react@07f46ecf2 breaks breaks tests - facebook/react@6fb8133ed breaks fbsource tests. I added a workaround and created a test for the team that owns the test. - https://fb.workplace.com/groups/flowlang/permalink/1198137807458547/ enables local type interference in fbsource but not in github React repo and some code breaks. Addressed in facebook/react#26064 This sync includes the following changes: - **[17f6912a4](facebook/react@17f6912a4 )**: Add flow types to ReactFiberHooks ([#25752](facebook/react#25752)) //<Samuel Susla>// - **[f101c2d0d](facebook/react@f101c2d0d )**: Remove Reconciler fork (2/2) ([#25775](facebook/react#25775)) //<Jan Kassens>// - **[420f0b7fa](facebook/react@420f0b7fa )**: Remove Reconciler fork (1/2) ([#25774](facebook/react#25774)) //<Jan Kassens>// - **[3ba7add60](facebook/react@3ba7add60 )**: Allow async blocks in `to(Error|Warn)Dev` ([#25338](facebook/react#25338)) //<Sebastian Silbermann>// - **[fa11bd6ec](facebook/react@fa11bd6ec )**: [ServerRenderer] Add option to send instructions as data attributes ([#25437](facebook/react#25437)) //<mofeiZ>// - **[e98225485](facebook/react@e98225485 )**: Add ref cleanup function ([#25686](facebook/react#25686)) //<Samuel Susla>// - **[15557fa67](facebook/react@15557fa67 )**: [Fix] properly track `useId` use in StrictMode in development ([#25713](facebook/react#25713)) //<Josh Story>// - **[8a23def32](facebook/react@8a23def32 )**: Resubmit Add HydrationSyncLane ([#25711](facebook/react#25711)) //<Tianyu Yao>// - **[2655c9354](facebook/react@2655c9354 )**: Fizz Browser: fix precomputed chunk being cleared on Node 18 ([#25645](facebook/react#25645)) //<Jimmy Lai>// - **[c08d8b804](facebook/react@c08d8b804 )**: Revert "Add SyncHydrationLane" ([#25708](facebook/react#25708)) //<Tianyu Yao>// - **[56ffca8b9](facebook/react@56ffca8b9 )**: Add Bun streaming server renderer ([#25597](facebook/react#25597)) //<Colin McDonnell>// - **[f31005d6a](facebook/react@f31005d6a )**: Add SyncHydrationLane ([#25698](facebook/react#25698)) //<Tianyu Yao>// - **[f284d9faf](facebook/react@f284d9faf )**: Track ThenableState alongside other hooks //<Andrew Clark>// - **[6b4c0314e](facebook/react@6b4c0314e )**: Check thenable instead of thenableState //<Andrew Clark>// - **[33e3d2878](facebook/react@33e3d2878 )**: Reuse hooks when replaying a suspended component //<Andrew Clark>// - **[4387d752d](facebook/react@4387d752d )**: Allow more hooks to be added when replaying mount //<Andrew Clark>// - **[5eb78d0a0](facebook/react@5eb78d0a0 )**: Pass ThenableState to replaySuspendedUnitOfWork //<Andrew Clark>// - **[4a2d86bdd](facebook/react@4a2d86bdd )**: Don't reset work loop until stack is unwound //<Andrew Clark>// - **[9dfbd9fa9](facebook/react@9dfbd9fa9 )**: use: Don't suspend if there are pending updates //<Andrew Clark>// - **[44c4e6f4d](facebook/react@44c4e6f4d )**: Force unwind work loop during selective hydration ([#25695](facebook/react#25695)) //<Andrew Clark>// - **[7b17f7bbf](facebook/react@7b17f7bbf )**: Enable warning for defaultProps on function components for everyone ([#25699](facebook/react#25699)) //<Sebastian Markbåge>// - **[6fb8133ed](facebook/react@6fb8133ed )**: Turn on string ref deprecation warning for everybody (not codemoddable) ([#25383](facebook/react#25383)) //<Sebastian Silbermann>// - **[07f46ecf2](facebook/react@07f46ecf2 )**: Turn on key spread warning in jsx-runtime for everyone ([#25697](facebook/react#25697)) //<Sebastian Markbåge>// - **[d65b88d03](facebook/react@d65b88d03 )**: Eagerly initialize an mutable object for instance.refs ([#25696](facebook/react#25696)) //<Sebastian Markbåge>// - **[c343f8025](facebook/react@c343f8025 )**: [react-float] feature detect getRootNode ([#25689](facebook/react#25689)) //<Jan Kassens>// - **[e1dd0a2f5](facebook/react@e1dd0a2f5 )**: Remove recoverable error when a sync update flows into a dehydrated boundary ([#25692](facebook/react#25692)) //<Sebastian Markbåge>// - **[c54e3541b](facebook/react@c54e3541b )**: [DevTools] bug fix for Hydrating fibers ([#25663](facebook/react#25663)) //<Mengdi Chen>// Changelog: [General][Changed] - React Native sync for revisions d1e35c7...17f6912 jest_e2e[run_all_tests] Reviewed By: makovkastar Differential Revision: D42804802 fbshipit-source-id: 6a9f00724cc73378025bbd04edb2d17760a87280

Summary: Three problems popped up during the sync: - facebook/react@07f46ecf2 breaks breaks tests - facebook/react@6fb8133ed breaks fbsource tests. I added a workaround and created a test for the team that owns the test. - https://fb.workplace.com/groups/flowlang/permalink/1198137807458547/ enables local type interference in fbsource but not in github React repo and some code breaks. Addressed in facebook/react#26064 This sync includes the following changes: - **[17f6912a4](facebook/react@17f6912a4 )**: Add flow types to ReactFiberHooks ([facebook#25752](facebook/react#25752)) //<Samuel Susla>// - **[f101c2d0d](facebook/react@f101c2d0d )**: Remove Reconciler fork (2/2) ([facebook#25775](facebook/react#25775)) //<Jan Kassens>// - **[420f0b7fa](facebook/react@420f0b7fa )**: Remove Reconciler fork (1/2) ([facebook#25774](facebook/react#25774)) //<Jan Kassens>// - **[3ba7add60](facebook/react@3ba7add60 )**: Allow async blocks in `to(Error|Warn)Dev` ([facebook#25338](facebook/react#25338)) //<Sebastian Silbermann>// - **[fa11bd6ec](facebook/react@fa11bd6ec )**: [ServerRenderer] Add option to send instructions as data attributes ([facebook#25437](facebook/react#25437)) //<mofeiZ>// - **[e98225485](facebook/react@e98225485 )**: Add ref cleanup function ([facebook#25686](facebook/react#25686)) //<Samuel Susla>// - **[15557fa67](facebook/react@15557fa67 )**: [Fix] properly track `useId` use in StrictMode in development ([facebook#25713](facebook/react#25713)) //<Josh Story>// - **[8a23def32](facebook/react@8a23def32 )**: Resubmit Add HydrationSyncLane ([facebook#25711](facebook/react#25711)) //<Tianyu Yao>// - **[2655c9354](facebook/react@2655c9354 )**: Fizz Browser: fix precomputed chunk being cleared on Node 18 ([facebook#25645](facebook/react#25645)) //<Jimmy Lai>// - **[c08d8b804](facebook/react@c08d8b804 )**: Revert "Add SyncHydrationLane" ([facebook#25708](facebook/react#25708)) //<Tianyu Yao>// - **[56ffca8b9](facebook/react@56ffca8b9 )**: Add Bun streaming server renderer ([facebook#25597](facebook/react#25597)) //<Colin McDonnell>// - **[f31005d6a](facebook/react@f31005d6a )**: Add SyncHydrationLane ([facebook#25698](facebook/react#25698)) //<Tianyu Yao>// - **[f284d9faf](facebook/react@f284d9faf )**: Track ThenableState alongside other hooks //<Andrew Clark>// - **[6b4c0314e](facebook/react@6b4c0314e )**: Check thenable instead of thenableState //<Andrew Clark>// - **[33e3d2878](facebook/react@33e3d2878 )**: Reuse hooks when replaying a suspended component //<Andrew Clark>// - **[4387d752d](facebook/react@4387d752d )**: Allow more hooks to be added when replaying mount //<Andrew Clark>// - **[5eb78d0a0](facebook/react@5eb78d0a0 )**: Pass ThenableState to replaySuspendedUnitOfWork //<Andrew Clark>// - **[4a2d86bdd](facebook/react@4a2d86bdd )**: Don't reset work loop until stack is unwound //<Andrew Clark>// - **[9dfbd9fa9](facebook/react@9dfbd9fa9 )**: use: Don't suspend if there are pending updates //<Andrew Clark>// - **[44c4e6f4d](facebook/react@44c4e6f4d )**: Force unwind work loop during selective hydration ([facebook#25695](facebook/react#25695)) //<Andrew Clark>// - **[7b17f7bbf](facebook/react@7b17f7bbf )**: Enable warning for defaultProps on function components for everyone ([facebook#25699](facebook/react#25699)) //<Sebastian Markbåge>// - **[6fb8133ed](facebook/react@6fb8133ed )**: Turn on string ref deprecation warning for everybody (not codemoddable) ([facebook#25383](facebook/react#25383)) //<Sebastian Silbermann>// - **[07f46ecf2](facebook/react@07f46ecf2 )**: Turn on key spread warning in jsx-runtime for everyone ([facebook#25697](facebook/react#25697)) //<Sebastian Markbåge>// - **[d65b88d03](facebook/react@d65b88d03 )**: Eagerly initialize an mutable object for instance.refs ([facebook#25696](facebook/react#25696)) //<Sebastian Markbåge>// - **[c343f8025](facebook/react@c343f8025 )**: [react-float] feature detect getRootNode ([facebook#25689](facebook/react#25689)) //<Jan Kassens>// - **[e1dd0a2f5](facebook/react@e1dd0a2f5 )**: Remove recoverable error when a sync update flows into a dehydrated boundary ([facebook#25692](facebook/react#25692)) //<Sebastian Markbåge>// - **[c54e3541b](facebook/react@c54e3541b )**: [DevTools] bug fix for Hydrating fibers ([facebook#25663](facebook/react#25663)) //<Mengdi Chen>// Changelog: [General][Changed] - React Native sync for revisions d1e35c7...17f6912 jest_e2e[run_all_tests] Reviewed By: makovkastar Differential Revision: D42804802 fbshipit-source-id: 6a9f00724cc73378025bbd04edb2d17760a87280

zfm-lucaschultz · 2024-03-06T10:25:24Z

Hi, sorry for the question, but since this is a pretty small change, I couldn't find any information on this anywhere else.

Am I mistaken in assuming that this hasn't been released yet? I can't seem to find any mention of it in either the changelog or in the canary changelog. @rickhanlonii has mentioned in a comment that it has landed in canary. I would love to get more information about this because "in the canary" might mean I could already use it in Next.js.

Resources - RFC: reactjs/rfcs#205 - Warning implemented in #22313 - Warning enabled in #23145 - Feature added in #25686 We have warned to prevent the old behavior since 18.0.0. The new feature has been on in canary for a while but still triggering the warning. This PR cleans up the warning for 19

Resources - RFC: reactjs/rfcs#205 - Warning implemented in #22313 - Warning enabled in #23145 - Feature added in #25686 We have warned to prevent the old behavior since 18.0.0. The new feature has been on in canary for a while but still triggering the warning. This PR cleans up the warning for 19 DiffTrain build for commit db913d8.

Resources - RFC: reactjs/rfcs#205 - Warning implemented in #22313 - Warning enabled in #23145 - Feature added in #25686 We have warned to prevent the old behavior since 18.0.0. The new feature has been on in canary for a while but still triggering the warning. This PR cleans up the warning for 19 DiffTrain build for [db913d8](db913d8)

yungsters · 2024-10-22T18:16:15Z

packages/react-reconciler/src/ReactFiberCommitWork.new.js

+        current.refCleanup = null;
+        const finishedWork = current.alternate;
+        if (finishedWork != null) {
+          finishedWork.refCleanup = null;


Why do we set refCleanup to null? What is finishedWork? Is it possible that finishedWork contains a ref that we have not yet detached?

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Nov 15, 2022

sammy-SC force-pushed the add-ref-cleanup-function branch 2 times, most recently from d8e5b2d to 608d9b6 Compare November 15, 2022 18:43

sammy-SC force-pushed the add-ref-cleanup-function branch from 608d9b6 to 71a6323 Compare November 16, 2022 15:16

sammy-SC added 3 commits November 16, 2022 15:33

Add ref cleanup function

3716c39

Delete commitDetachRef

be50757

yarn prettier

073cec2

sammy-SC force-pushed the add-ref-cleanup-function branch from 55e2c29 to 073cec2 Compare November 16, 2022 15:33

Move cleanup to finally branch

b9a8ed7

sammy-SC marked this pull request as ready for review November 16, 2022 15:47

sammy-SC requested review from acdlite, gaearon, rickhanlonii and sebmarkbage November 16, 2022 15:47

sebmarkbage reviewed Nov 16, 2022

View reviewed changes

sebmarkbage approved these changes Nov 16, 2022

View reviewed changes

gaearon mentioned this pull request Nov 16, 2022

RFC: Callback Ref Cleanup reactjs/rfcs#205

Open

Remove typeof check

9d315ab

sammy-SC added 2 commits November 17, 2022 13:20

Add unit test to cover conditionally returned clean up function

d68d6e9

Remove null check

ebb8afc

sammy-SC merged commit e982254 into facebook:main Nov 29, 2022

sammy-SC deleted the add-ref-cleanup-function branch November 29, 2022 16:41

rickhanlonii mentioned this pull request Jan 8, 2024

React callback ref cleanup function #15176

Open

eps1lon mentioned this pull request Mar 20, 2024

[react] Add types for ref cleanup functions to Canary release channel DefinitelyTyped/DefinitelyTyped#69065

Merged

jackpope mentioned this pull request Apr 19, 2024

Remove warning for ref cleanup function #28883

Merged

yungsters reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ref cleanup function #25686

Add ref cleanup function #25686

sammy-SC commented Nov 15, 2022 •

edited

Loading

sizebot commented Nov 15, 2022 •

edited

Loading

sebmarkbage Nov 16, 2022

sebmarkbage Nov 16, 2022

sebmarkbage left a comment •

edited

Loading

sebmarkbage left a comment

gaearon commented Nov 16, 2022 •

edited

Loading

sebmarkbage commented Nov 16, 2022

KurtGokhan commented Nov 16, 2022

gaearon commented Nov 17, 2022

KurtGokhan commented Nov 17, 2022

gaearon commented Nov 17, 2022

dante01yoon commented Nov 30, 2022

zfm-lucaschultz commented Mar 6, 2024

yungsters Oct 22, 2024

Add ref cleanup function #25686

Add ref cleanup function #25686

Conversation

sammy-SC commented Nov 15, 2022 • edited Loading

sizebot commented Nov 15, 2022 • edited Loading

Critical size changes

Significant size changes

sebmarkbage Nov 16, 2022

Choose a reason for hiding this comment

sebmarkbage Nov 16, 2022

Choose a reason for hiding this comment

sebmarkbage left a comment • edited Loading

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

gaearon commented Nov 16, 2022 • edited Loading

sebmarkbage commented Nov 16, 2022

KurtGokhan commented Nov 16, 2022

gaearon commented Nov 17, 2022

KurtGokhan commented Nov 17, 2022

gaearon commented Nov 17, 2022

dante01yoon commented Nov 30, 2022

zfm-lucaschultz commented Mar 6, 2024

yungsters Oct 22, 2024

Choose a reason for hiding this comment

sammy-SC commented Nov 15, 2022 •

edited

Loading

sizebot commented Nov 15, 2022 •

edited

Loading

sebmarkbage left a comment •

edited

Loading

gaearon commented Nov 16, 2022 •

edited

Loading