Add useMergeRefs hook #27768
Merged
Add useMergeRefs hook #27768
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ellatrix
force-pushed
the
try/use-merge-refs
branch
from
9c24593
to
457db3a
Compare
|
Size Change: -415 B (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Dec 16, 2020
| // - The value changes. | ||
| // - The ref callback has changed (e.g. an updated dependency). | ||
| if ( | ||
| value !== lastValue.current || |
There was a problem hiding this comment.
I believe comparing to the previous value is not useful because if one of the ref changes, React will first call the old function with "null" and then call the new function with the new node which mean this check is always true which also means that the implementation here is equivalent to useCallback( merge(refs), refs ) and in both situations we can't have a callback with dependencies as ref because if it changes, all the other merged refs, even the ones that didn't change,w ill be called again (creating memory leaks or unexpected behavior)
There was a problem hiding this comment.
all the other merged refs, even the ones that didn't change,w ill be called again
No, they won't be called since we check if the callback has been changed. Only if it's changed we call it, and we leave any others that remain the same alone. The value check is there to ensure that, even if the callback didn't change, the ref callback is still called.
There was a problem hiding this comment.
Added clearer documentation in ece1982. I tested ref callbacks with and without dependencies and they both work as expected. If a dependency changes, old callback is called with null, the new with the node. If no dependency changes, the callback is never called, even if other ref callback dependencies change.
There was a problem hiding this comment.
I'll try to add some test cases tomorrow.
There was a problem hiding this comment.
Here's the failing test case:
const ref1 = useCallback( dosomething, [dep1]);
const ref2 = useCallback( dosomething2, [] );
const ref = useMergeRef( [ ref1, ref2 ] );
if the node doesn't change and dep1 change, dosomething2 will be called again while it shouldn't be. (I didn't test but that's my read of the code) Let's add this unit test and see.
There was a problem hiding this comment.
You're right, it's calling the wrong callback. Changed the code and started adding tests, but it's still a work in progress.
|
@youknowriad I think I figured it out now. The new approach seems to work well in tests. |
6 tasks
|
Noting that I had issues when upgrading I like the idea of replacing this 10 lines long utility with a hook that has additional optimizations 👍🏻 |
gziolo
added
[Type] New API
|
One of the failing e2e tests is concerning:
I'm wondering it wasn't a temporary issue elsewhere. |
6 tasks
6 tasks
ellatrix
force-pushed
the
try/use-merge-refs
branch
from
8e82d20
to
b3d1a12
Compare
|
Merging since we need this for several PRs that want to add dependencies to ref callbacks, which will currently cause the callbacks to be called too many times. |
oandregal
pushed a commit
that referenced
this pull request
Feb 11, 2021
* Try: useMergeRefs * update lock file * Rebase fixes * Add docs * Clearer documentation * wip * wip * Add docs for unit tests * Convert new additions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
See #27675 (comment).
This
useMergeRefshook would allow you to merge refs even if any if any ref callbacks have dependencies. The trick is to have the wrapping ref called as many times as all the dependencies combined, but to check for each ref individually if it wants to be called, that is, when the value of the ref changed or when the ref callback itself changed.The problem with the external
mergeRefsfunction is that is calls all merged callbacks on every render and doesn't respect refs wrapped inuseCallback.How has this been tested?
Screenshots
Types of changes
Checklist: