[react-interactions] Move Flare event registration to commit phase #17518
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
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c8794a0:
|
sebmarkbage
approved these changes
Dec 4, 2019
Details of bundled changes.Comparing: 54f6673...c8794a0 react-native-renderer
react-test-renderer
react-art
react-dom
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 54f6673...c8794a0 react-native-renderer
Size changes (stable) |
trueadm
added a commit
to trueadm/react
that referenced
this pull request
Dec 4, 2019
trueadm
added a commit
to trueadm/react
that referenced
this pull request
Dec 4, 2019
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.
This PR fixes an issue that can lead to memory leaks with the Flare event system. Specifically, the Flare event system has the notion of "root events" that are meant to be triggered when an incoming dispatched event gets bubbled to the document. Upon hitting the document, we then check a global Map, to see if there are any active event responders listening to the given type of event coming through.
The issue is that this Map can't be GC'd when DOM nodes are detached and removed, because we need to iterate over the Map's values to get all active responders that listen to root events. Normally, this wouldn't be an issue, but because of the heuristics as to how React reconciles responders differently depending on initial render vs update render, the root events can be subscribed to, but never used due to unwinding and other characteristics within the reconciliation processes. The expectation is that things that occur in initial render should be effects that can be unwound simply due to the nature of the garbage collector, but because of the nature of how root events work – this will never be the case (we need to be able to iterate over root events on any given incoming dispatch event, which means we can't use a WeakMap or WeakSet).
In the ideal world, no root event listeners should be created on initial render. Unfortunately, this is likely to be a more complex and feature breaking change to make at this point given the requirements of the Focus and FocusWithin event responders, which both make use of root events on initial mount. This PR tackles the problem in a somewhat brute-force approach, by moving all React Flare responders to the commit phase, at the compromise of more commit phase work vs the benefit of no leaking of the root event Map.
I believe this is probably the best short-term solution, and a lesson we can take forward in ironing out kinks with the newer Listener API. We can also, in the short-term, possibly come up with a way of remove root events from event responders on initial mount and finding a way to make them work conditionally, but this will likely take a larger period of time. In the interests of unblocking this memory leak, this seems the most sane path forward at this point.