[Events] Make passiveness and priority non-configurable #19807
Merged
Conversation
This file contains hidden or 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 8955b5e:
|
Details of bundled changes.Comparing: cd75f93...8955b5e react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: cd75f93...8955b5e react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
gaearon commented Sep 10, 2020
| let elementListenerMap = (node: any)[internalEventHandlersKey]; | ||
| if (elementListenerMap === undefined) { | ||
| elementListenerMap = (node: any)[internalEventHandlersKey] = new Map(); | ||
| export function getEventListenerSet(node: EventTarget): ElementListenerSet { |
Collaborator Author
There was a problem hiding this comment.
No longer needs to be a Map because we won't read listeners.
| false, | ||
| 'ReactDOM.createEventHandle: setListener called on an target ' + | ||
| 'that did not have a corresponding root. This is likely a bug in React.', | ||
| if (!enableEagerRootListeners) { |
Collaborator Author
There was a problem hiding this comment.
In eager mode, we attach to roots early, so no need to do it here anymore.
| // any code relying on the createEventHandle API. | ||
| invariant( | ||
| allNativeEvents.has(domEventName) || | ||
| domEventName === 'beforeblur' || |
Collaborator Author
There was a problem hiding this comment.
I've needed to put them into the list so that we actually listen to them.
| listenerPriority, | ||
| ); | ||
| // Add the event to our known event types list. | ||
| addEventTypeToDispatchConfig(domEventName); |
Collaborator Author
There was a problem hiding this comment.
No need to do this now since we've removed support for arbitrary events for now.
| processDispatchQueue(dispatchQueue, eventSystemFlags); | ||
| } | ||
| | ||
| function shouldUpgradeListener( |
Collaborator Author
There was a problem hiding this comment.
We don't need this mechanism anymore.
| ) { | ||
| target = (rootContainerElement: any).ownerDocument; | ||
| } | ||
| if (enablePassiveEventIntervention && isPassiveListener === undefined) { |
Collaborator Author
There was a problem hiding this comment.
This moved below since we can now decide it right before attaching the listener.
| priority === undefined | ||
| ? getEventPriorityForPluginSystem(domEventName) | ||
| : priority; | ||
| const eventPriority = getEventPriorityForPluginSystem(domEventName); |
Collaborator Author
There was a problem hiding this comment.
React always decides the priority now.
gaearon force-pushed the fewer-options branch 3 times, most recently from 1d5611c to b2eb0f5 Compare 
facebook-github-bot added CLA Signed React Core Team 
trueadm approved these changes Sep 10, 2020
gaearon force-pushed the fewer-options branch 2 times, most recently from 089247e to af8e53b Compare 
gaearon force-pushed the fewer-options branch from af8e53b to 8955b5e Compare todortotev pushed a commit to todortotev/react that referenced this pull request Sep 18, 2020
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
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.
These are already non-configurable in public APIs, but they are configurable in
createEventHandle. This introduces complexity for all paths, such as very hot normal event paths, but is only used forcreateEventHandle. Even forcreateEventHandle, this is not ideal because we're continually hitting these Map checks and can't benefit from eager listeners.In this PR, I'm removing support for the options argument beyond
capture. This lets us remove the Map checks every timesetHandleis called — nowcreateEventHandlealso relies on the eager listeners. It also lets us remove the upgrade mechanism. It also lets us speed up thecreateEventHandlepath because we don't need to search for Fibers or the closest containers. (I've turned invariants into warnings.)Overall, the goal is to improve perf and reduce the API surface area for something we're not yet committed to supporting. We can revisit this after the planned changes to event defaults and the event replaying refactor.