[react-dom] Refactor event priority handling to its own module#17678
Merged
trueadm merged 1 commit intofacebook:masterfrom Dec 20, 2019
Merged
[react-dom] Refactor event priority handling to its own module#17678trueadm merged 1 commit intofacebook:masterfrom
trueadm merged 1 commit intofacebook:masterfrom
Conversation
trueadm
requested review from
acdlite,
gaearon,
necolas,
philipp-spiess and
sebmarkbage
|
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 bf1ed10:
|
Details of bundled changes.Comparing: dbc46ac...bf1ed10 react-dom
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: dbc46ac...bf1ed10 react-dom
ReactDOM: size: -0.4%, gzip: -0.6% Size changes (stable) |
trueadm
force-pushed
the
event-priority-refactor
branch
from
2f6b4be to
5ef8236
Compare
necolas
approved these changes
Dec 20, 2019
trueadm
force-pushed
the
event-priority-refactor
branch
2 times, most recently
from
7059476 to
5c4998d
Compare
Contributor
Author
|
I did some benchmarking on FF and Safari and it looks like using multiple Sets is not as optimal as Chrome's V8. I've changed my approach to use Maps and moved things around so we don't do multiple lookups – this is now consistently faster in Chrome and Safari, neutral in Firefox. I also did some neat code golfing to save us more code size compared to master! |
trueadm
force-pushed
the
event-priority-refactor
branch
2 times, most recently
from
c292316 to
e12b4bf
Compare
necolas
reviewed
Dec 20, 2019
Contributor
necolas
left a comment
necolas
left a comment
There was a problem hiding this comment.
Can we avoid creating the cyclic dependency?
| priority: EventPriority, | ||
| ): void { | ||
| // As the event types are in pairs of two, we need to iterate | ||
| // through in twos. |
trueadm
force-pushed
the
event-priority-refactor
branch
3 times, most recently
from
f6634dc to
0df550e
Compare
necolas
reviewed
Dec 20, 2019
Fix lint Revise comment Revise strategy Did some code golfing Added comment about iteration Remove cyclic dependency Fix typo Extend comment Fix typo
trueadm
force-pushed
the
event-priority-refactor
branch
from
0df550e to
bf1ed10
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
The team have mentioned a bunch of times that it felt wrong that DOM Event Priority handling was all coupled with the SimpleEventPlugin, especially as might want to extend on the priorities with newer features and refactors in the future.
This PR breaks out the priority logic into a dedicated module and makes uses of Maps for faster lookups (I benchmarked before and after and Maps were consistently faster than object property lookups in Chrome and Safari, neutral in Firefox). We should also get a slight code size saving by some of the code golfing that I did whilst refactoring the code.