[Events] Make passiveness and priority non-configurable #19807
Merged
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 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 { |
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) { |
There was a problem hiding this comment.
In eager mode, we attach to roots early, so no need to do it here anymore.
| @@ -192,35 +184,18 @@ export function createEventHandle( | |||
| // event from the list of known events has now become a breaking change for | |||
| // any code relying on the createEventHandle API. | |||
| invariant( | |||
| allNativeEvents.has(domEventName) || | |||
| domEventName === 'beforeblur' || | |||
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); |
There was a problem hiding this comment.
No need to do this now since we've removed support for arbitrary events for now.
| @@ -296,36 +293,24 @@ function dispatchEventsForPlugins( | |||
| processDispatchQueue(dispatchQueue, eventSystemFlags); | |||
| } | |||
|
|
|||
| function shouldUpgradeListener( | |||
There was a problem hiding this comment.
We don't need this mechanism anymore.
| @@ -384,21 +367,6 @@ export function listenToNativeEvent( | |||
| ) { | |||
| target = (rootContainerElement: any).ownerDocument; | |||
| } | |||
| if (enablePassiveEventIntervention && isPassiveListener === undefined) { | |||
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); |
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
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.