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

Simplify discreteUpdates #21773

Merged
merged 1 commit into from
Jun 30, 2021
Merged

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jun 30, 2021

Now that discrete updates are flushed synchronously in a microtask, the discreteUpdates method used by our event system is only an optimization to save us from having to check window.event.type on every update. So we should be able to remove the extra logic.

Assuming this lands successfully, we can remove batchedEventUpdates and probably inline discreteUpdates into the renderer, like we do for continuous updates.

Now that discrete updates are flushed synchronously in a microtask,
the `discreteUpdates` method used by our event system is only a
optimization to save us from having to check `window.event.type` on
every update. So we should be able to remove the extra logic.

Assuming this lands successfully, we can remove `batchedEventUpdates`
and probably inline `discreteUpdates` into the renderer, like we do
for continuous updates.
@facebook-github-bot facebook-github-bot added React Core Team Opened by a member of the React Core Team CLA Signed labels Jun 30, 2021
@sizebot
Copy link

sizebot commented Jun 30, 2021

Comparing: 3e8c86c...6cce423

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 = 127.41 kB 127.20 kB = 40.83 kB 40.77 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.22 kB 130.01 kB = 41.74 kB 41.67 kB
facebook-www/ReactDOM-prod.classic.js = 405.65 kB 404.75 kB = 74.99 kB 74.83 kB
facebook-www/ReactDOM-prod.modern.js = 394.08 kB 393.15 kB = 73.22 kB 73.07 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.65 kB 404.75 kB = 75.00 kB 74.83 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-profiling.classic.js = 430.79 kB 429.89 kB = 79.30 kB 79.12 kB
facebook-www/ReactDOMForked-profiling.classic.js = 430.79 kB 429.89 kB = 79.30 kB 79.12 kB
facebook-www/ReactDOM-profiling.modern.js = 419.17 kB 418.24 kB = 77.54 kB 77.38 kB
facebook-www/ReactDOMForked-profiling.modern.js = 419.17 kB 418.24 kB = 77.54 kB 77.38 kB
facebook-www/ReactDOM-prod.classic.js = 405.65 kB 404.75 kB = 74.99 kB 74.83 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.65 kB 404.75 kB = 75.00 kB 74.83 kB
facebook-www/ReactDOMTesting-prod.classic.js = 396.97 kB 396.07 kB = 74.84 kB 74.67 kB
facebook-www/ReactDOM-prod.modern.js = 394.08 kB 393.15 kB = 73.22 kB 73.07 kB
facebook-www/ReactDOMForked-prod.modern.js = 394.08 kB 393.15 kB = 73.23 kB 73.07 kB
facebook-www/ReactDOMTesting-prod.modern.js = 383.71 kB 382.78 kB = 72.70 kB 72.53 kB

Generated by 🚫 dangerJS against 6cce423

@acdlite acdlite merged commit ae5afb3 into facebook:main Jun 30, 2021
@sebmarkbage
Copy link
Collaborator

I'm not sure how the description and the change are related. Discrete updates flushing in a micro-task doesn't apply to legacy mode and so shared code paths still need to be in place.

Also, for controlled inputs which is when the timing of flushing finishEventHandler needs to happen before the promise next tick anyway I think.

The simplification makes sense though.

@acdlite
Copy link
Collaborator Author

acdlite commented Jun 30, 2021

I mentioned microtasks because we don't bother to prevent overflushing anymore, because flushDiscreteUpdates doesn't exist conceptually, because we either do it at the end of the event (in a microtask or in finishEventHandler) or with flushSync. Instead of the previous mechanism, which was do it at the beginning of every discrete event, even if it's nested.

That's why I could remove batchedEventUpdates, too, which was added here originally to prevent overflushing: #15687

zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
Now that discrete updates are flushed synchronously in a microtask,
the `discreteUpdates` method used by our event system is only a
optimization to save us from having to check `window.event.type` on
every update. So we should be able to remove the extra logic.

Assuming this lands successfully, we can remove `batchedEventUpdates`
and probably inline `discreteUpdates` into the renderer, like we do
for continuous updates.
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