Skip to content

Refactor Enter/Leave listener accumulation#18405

Merged
trueadm merged 2 commits intofacebook:masterfrom
trueadm:enter-leave
Apr 2, 2020
Merged

Refactor Enter/Leave listener accumulation#18405
trueadm merged 2 commits intofacebook:masterfrom
trueadm:enter-leave

Conversation

@trueadm
Copy link
Copy Markdown
Contributor

@trueadm trueadm commented Mar 27, 2020

We already cleaned up and refactored two phase event accumulation in a previous PR, this PR cleans up and refactors the Enter/Leave logic so it's easier to follow and adapt for future work where we can refactor how events are accumulated and ties to synthetic events. This should also have a slight runtime performance win as we don't accumulate as many objects with this implementation.

I also inlined all the rest of the logic into RN and ResponderPlugin, as we no longer need a bunch of code for ReactDOM.

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Mar 27, 2020

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.

@sizebot
Copy link
Copy Markdown

sizebot commented Mar 27, 2020

Warnings
⚠️ Could not find build artifacts for base commit: d6d5f5a

Size changes (stable)

Generated by 🚫 dangerJS against 50c5e24

@sizebot
Copy link
Copy Markdown

sizebot commented Mar 27, 2020

Warnings
⚠️ Build job for base commit is still pending: d6d5f5a

Size changes (experimental)

Generated by 🚫 dangerJS against 50c5e24

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 27, 2020
Comment thread packages/react-dom/src/events/accumulateEnterLeaveListeners.js
@trueadm trueadm force-pushed the enter-leave branch 3 times, most recently from da1a743 to f61c832 Compare March 27, 2020 18:12
Comment thread packages/legacy-events/ResponderEventPlugin.js Outdated
@trueadm
Copy link
Copy Markdown
Contributor Author

trueadm commented Mar 30, 2020

This also slightly reduces the prod size of ReactDOM by the looks of it (although not by much, 0.1%). @gaearon @necolas let me know if there's anything else that sticks out to you both please?

@gaearon
Copy link
Copy Markdown
Collaborator

gaearon commented Mar 31, 2020

Let’s wait a bit before we figure out the bug in the recent event commit. Caught it after a sync.

@trueadm trueadm force-pushed the enter-leave branch 5 times, most recently from e6ea83a to b6267f4 Compare March 31, 2020 18:21
Copy link
Copy Markdown
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

// EventPropagator.js, as they deviated from ReactDOM's newer
// implementations.

function getParent(inst) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that this is 'legacy', but it's weird that fibers are called instances here (since we use that word for something else elsewhere.

could you explain why this other traversal fns are inlined multiple times instead of in one module? Also why do some of them have types, and some don't?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're inlined because we don't want them in ReactDOM anymore and by pulling them out, it means we won't accidently break another module by changing something else. They're essentially legacy, and in a bunch of the cases, will likely be killed when their associated module drops them. Some have types because the module is Flow types, whilst before they came from a module that wasn't Flow typed.

@trueadm trueadm merged commit 663c13d into facebook:master Apr 2, 2020
@trueadm trueadm deleted the enter-leave branch April 2, 2020 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants