Ref merging: fix memory leak #27686
Closed
Ref merging: fix memory leak #27686
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
added
[Type] Bug
6 tasks
|
Size Change: +52 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Dec 11, 2020
youknowriad
reviewed
Dec 11, 2020
| * @return {Function} A new ref callback. | ||
| */ | ||
| export default function useMergeRefs( ...refs ) { | ||
| return useCallback( mergeRefs( refs ), refs ); |
There was a problem hiding this comment.
Actually, thinking more, the addition of "refs" as a dependency is not a good thing. If one of the refs change, all the refs will be recalled again even the ones that didn't change. Not sure exactly how to solve this. We could solve this by "convention".
Meaning we make sure we only use useMergeRefs for refs that don't change over time (so even the useCallback we use right now for these refs is not necessary. It doesn't sound great though, so I'm wondering if there's a smarter solution (only call the ref function again if it changes). I also wonder whether such thing exists in npm already.
There was a problem hiding this comment.
I agree, I'm not sure if it a good thing, because on the one hand there's the assumption that a ref callback shouldn't be called if there's no new node, and on the other hand, there's the dependency issue, which you've resolved before by setting the dependencies as refs.
I think we should always return the same callback throughout the lifecycle as refs are supposed to behave.
There was a problem hiding this comment.
Another way is to keep using mergeRefs like today and just "memoize" the ref callbacks. I think it might be the best way. We could make it a hook, useMemoizeCallback or something like that :P
There was a problem hiding this comment.
You mean replace useCallback with useMemo?
There was a problem hiding this comment.
no, I mean const ref = useCallback( memize( (node) => { // do something } ), [ deps ] )
There was a problem hiding this comment.
Yes, that's what we want, but now we are adjusting to a ref callback not being "purely" called when the node changes (or is added or removed). To me the expectation I have with a ref callback is that it's never called multiple times with the same node.
There was a problem hiding this comment.
It seems to me "purely" called doesn't mean much. useMemo or useCallback do the same thing as memize behind the scenes. I don't really see the difference. The difference is in one case, the memorization is inside React itself while in the other case it's outside React.
There was a problem hiding this comment.
I still think there's benefit in wrapping the function in useCallback, and add the refs as dependencies, which makes the makes all the inner useCallbacks work correctly. What's the point otherwise of wrapping ref callbacks in useCallback?
There was a problem hiding this comment.
the point is that the a new function is generated for the ref when one of the deps change which calls the callback again with the node, allowing you to react properly to these deps changes.
ellatrix
force-pushed
the
fix/ref-memory-leak
branch
from
02041d7
to
886c5a8
Compare
6 tasks
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
Currently, in
useConstrainedTabbing(and probably other hooks returning a ref), code is executed multiple times on the same node. InuseConstrainedTabbingthis results in event listeners being added multiple times.It's assumed that a ref callback will only be called when the node changes or the component unmounts, but that's not the case if callbacks are merged with
mergeRefsand directly passed as the ref.mergeRefsreturns a new function on every render, while it should return a persistent callback.How has this been tested?
In
master, log thenoderight beforenode.addEventListenerin theuseConstrainedTabbinghook. Open the editor, then "Options" in the top right corner and then click "Preferences" to open a modal. Observe that the modal node is logged twice, which means the event listener is added twice.Now check out this branch and follow the steps again. The modal node will only be logged once.
Screenshots
Types of changes
Checklist: