Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple dispatching from attemptToDispatchEvent #22851

Merged
merged 2 commits into from
Dec 2, 2021

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 1, 2021

Purely refactoring, no functional changes.

We need to split up dispatching from finding the blocked instance, so that we can apply different logic in each case (e.g. replay instead of dispatching through the event system). In #22680 we tried to do this via a special return type, but it's really noisy and I find it difficult to verify the existing behavior is intact. This is an alternative proposal, where we just use a few fields as makeshift "return values" to avoid allocating. I think I've seen this somewhere in the code. (I mean, we could also just use a tuple but idk if this is controversial for some reason.)

If we land this I think we can make #22680 simpler and more self-contained.

@gaearon gaearon changed the title Decoupled dispatching from attemptToDispatchEvent Decouple dispatching from attemptToDispatchEvent Dec 1, 2021
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Dec 1, 2021
@sizebot
Copy link

sizebot commented Dec 1, 2021

Comparing: ca106a0...905ab99

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.11% 130.01 kB 130.16 kB +0.11% 41.57 kB 41.62 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.11% 134.77 kB 134.91 kB +0.10% 42.96 kB 43.01 kB
facebook-www/ReactDOM-prod.classic.js +0.23% 424.63 kB 425.60 kB +0.10% 78.24 kB 78.32 kB
facebook-www/ReactDOM-prod.modern.js +0.23% 413.19 kB 414.16 kB +0.16% 76.51 kB 76.63 kB
facebook-www/ReactDOMForked-prod.classic.js +0.23% 424.63 kB 425.60 kB +0.09% 78.25 kB 78.32 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOM-prod.modern.js +0.23% 413.19 kB 414.16 kB +0.16% 76.51 kB 76.63 kB
facebook-www/ReactDOMForked-prod.modern.js +0.23% 413.19 kB 414.16 kB +0.16% 76.51 kB 76.63 kB
facebook-www/ReactDOM-prod.classic.js +0.23% 424.63 kB 425.60 kB +0.10% 78.24 kB 78.32 kB
facebook-www/ReactDOMForked-prod.classic.js +0.23% 424.63 kB 425.60 kB +0.09% 78.25 kB 78.32 kB
facebook-www/ReactDOM-profiling.modern.js +0.22% 445.02 kB 445.99 kB +0.16% 81.92 kB 82.05 kB
facebook-www/ReactDOMForked-profiling.modern.js +0.22% 445.02 kB 445.99 kB +0.16% 81.93 kB 82.05 kB
facebook-www/ReactDOM-profiling.classic.js +0.21% 456.51 kB 457.48 kB +0.09% 83.69 kB 83.76 kB
facebook-www/ReactDOMForked-profiling.classic.js +0.21% 456.51 kB 457.48 kB +0.08% 83.70 kB 83.77 kB

Generated by 🚫 dangerJS against 905ab99

// Attempt dispatching an event. Returns a SuspenseInstance or Container if it's blocked.
export function attemptToDispatchEvent(
export let return_shouldDispatch = false;
export let return_targetInst = null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Technically this leaks at module level but next event will clear it. So it's fine.

It is unnecessary because it's only true when retval is null.
@gaearon gaearon merged commit ea5a413 into facebook:main Dec 2, 2021
@gaearon gaearon deleted the decouple-ret branch December 2, 2021 13:56
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* Decoupled dispatching from attemptToDispatchEvent

* Remove unnecessary field

It is unnecessary because it's only true when retval is null.
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.

4 participants