Rewrite ReactFiberScheduler for better integration with Scheduler package #15151
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
acdlite
changed the title
|
Is it expected that React's size goes up by 1.4% gzip? :o I thought with these changes it would have gone down. Maybe missing some DEV wrappers? |
|
@trueadm All the changes I've made in this PR are in a forked file that isn't enabled yet (except via |
|
Once this PR is ready, I would expect ReactDOM's size to go slightly down, or at least stay even. Scheduler might go up a bit if I end up lifting stuff into that package. React isomorphic package shouldn't be affected at all. |
acdlite
force-pushed
the
root-scheduler
branch
5 times, most recently
from
50b7091
to
2d7a6ac
Compare
acdlite
force-pushed
the
root-scheduler
branch
2 times, most recently
from
fc0d30e
to
f007ae7
Compare
|
ReactDOM: size: 🔺+0.2%, gzip: 🔺+0.3% Details of bundled changes.Comparing: aed0e1c...f1dc626 react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
acdlite
force-pushed
the
root-scheduler
branch
5 times, most recently
from
d739dca
to
2f60c47
Compare
sebmarkbage
reviewed
Mar 30, 2019
acdlite
force-pushed
the
root-scheduler
branch
4 times, most recently
from
396abed
to
3b89f99
Compare
sebmarkbage
reviewed
Apr 1, 2019
packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js
Show resolved
Hide resolved
packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
Outdated
Show resolved
Hide resolved
packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
Outdated
Show resolved
Hide resolved
| @@ -1352,6 +1354,9 @@ describe('Profiler', () => { | |||
| }, | |||
| ); | |||
| }).toThrow('Expected error onWorkScheduled'); | |||
| if (enableNewScheduler) { | |||
| expect(Scheduler).toFlushAndYield(['Component:fail']); | |||
There was a problem hiding this comment.
What's happening here?
sebmarkbage
reviewed
Apr 1, 2019
|
Re: bundle size, I think it might be the |
sebmarkbage
approved these changes
Apr 2, 2019
Adds a new implementation of ReactFiberScheduler behind a feature flag. We will maintain both implementations in parallel until the new one is proven stable enough to replace the old one. The main difference between the implementations is that the new one is integrated with the Scheduler package's priority levels.
Some fields only used by the old scheduler, and some by the new.
If synchronous updates are scheduled by a passive effect, that work should be flushed synchronously, even if flushPassiveEffects is called inside batchedUpdates.
React doesn't need this anyway because it never schedules callbacks if it's already rendering.
Turns out this isn't neccessary.
Should initialize to nothing, then assign the exports conditionally, instead of initializing to the old exports and then reassigning to the new ones.
Instead of wrapping in enableNewScheduler flag.
NMinhNguyen
referenced
this pull request
in enzymejs/react-shallow-renderer
Jan 29, 2020
…kage (#15151) * Rewrite ReactFiberScheduler Adds a new implementation of ReactFiberScheduler behind a feature flag. We will maintain both implementations in parallel until the new one is proven stable enough to replace the old one. The main difference between the implementations is that the new one is integrated with the Scheduler package's priority levels. * Conditionally add fields to FiberRoot Some fields only used by the old scheduler, and some by the new. * Add separate build that enables new scheduler * Re-enable skipped test If synchronous updates are scheduled by a passive effect, that work should be flushed synchronously, even if flushPassiveEffects is called inside batchedUpdates. * Passive effects have same priority as render * Revert ability to cancel the current callback React doesn't need this anyway because it never schedules callbacks if it's already rendering. * Revert change to FiberDebugPerf Turns out this isn't neccessary. * Fix ReactFiberScheduler dead code elimination Should initialize to nothing, then assign the exports conditionally, instead of initializing to the old exports and then reassigning to the new ones. * Don't yield before commit during sync error retry * Call Scheduler.flushAll unconditionally in tests Instead of wrapping in enableNewScheduler flag.
sebmarkbage
added a commit
to sebmarkbage/react
that referenced
this pull request
Dec 10, 2020
We replay errors so you can break on paused exceptions. This is done in the second pass so that the first pass can ignore suspense. Originally this threw the original error. For suppression purposes we copied the flag onto the original error. https://github.com/facebook/react/blob/f1dc626b29b8bf0f14c75a8525e8650b7ea94a47/packages/react-reconciler/src/ReactFiberScheduler.old.js#L367-L369 During this refactor it changed to just throw the retried error: facebook#15151 We're not sure exactly why but it was likely just an optimization or clean up. So we can go back to throwing the original error. That helps in the case where a memoized function is naively not rethrowing each time such as in Node's module system.
sebmarkbage
added a commit
to sebmarkbage/react
that referenced
this pull request
Dec 10, 2020
We replay errors so you can break on paused exceptions. This is done in the second pass so that the first pass can ignore suspense. Originally this threw the original error. For suppression purposes we copied the flag onto the original error. https://github.com/facebook/react/blob/f1dc626b29b8bf0f14c75a8525e8650b7ea94a47/packages/react-reconciler/src/ReactFiberScheduler.old.js#L367-L369 During this refactor it changed to just throw the retried error: facebook#15151 We're not sure exactly why but it was likely just an optimization or clean up. So we can go back to throwing the original error. That helps in the case where a memoized function is naively not rethrowing each time such as in Node's module system. Unfortunately this doesn't fix the problem fully. Because invokeGuardedCallback captures the error and logs it to the browser. So you still end up seeing the wrong message in the logs. This just fixes so that the error boundary sees the first one.
sebmarkbage
added a commit
that referenced
this pull request
Dec 10, 2020
We replay errors so you can break on paused exceptions. This is done in the second pass so that the first pass can ignore suspense. Originally this threw the original error. For suppression purposes we copied the flag onto the original error. https://github.com/facebook/react/blob/f1dc626b29b8bf0f14c75a8525e8650b7ea94a47/packages/react-reconciler/src/ReactFiberScheduler.old.js#L367-L369 During this refactor it changed to just throw the retried error: #15151 We're not sure exactly why but it was likely just an optimization or clean up. So we can go back to throwing the original error. That helps in the case where a memoized function is naively not rethrowing each time such as in Node's module system. Unfortunately this doesn't fix the problem fully. Because invokeGuardedCallback captures the error and logs it to the browser. So you still end up seeing the wrong message in the logs. This just fixes so that the error boundary sees the first one.
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
We replay errors so you can break on paused exceptions. This is done in the second pass so that the first pass can ignore suspense. Originally this threw the original error. For suppression purposes we copied the flag onto the original error. https://github.com/facebook/react/blob/f1dc626b29b8bf0f14c75a8525e8650b7ea94a47/packages/react-reconciler/src/ReactFiberScheduler.old.js#L367-L369 During this refactor it changed to just throw the retried error: facebook#15151 We're not sure exactly why but it was likely just an optimization or clean up. So we can go back to throwing the original error. That helps in the case where a memoized function is naively not rethrowing each time such as in Node's module system. Unfortunately this doesn't fix the problem fully. Because invokeGuardedCallback captures the error and logs it to the browser. So you still end up seeing the wrong message in the logs. This just fixes so that the error boundary sees the first one.
facebook-github-bot
added
the
React Core Team
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.
Pushing this early so I won't lose my work if my computer combusts.
I'm not going to bother with splitting this into chunks until I get most/all of the tests passing. Then I'll work on getting it into a reviewable state. (It'll probably be hard to review regardless, though, given the nature of the PR.)
Intentional semantic differences
In the old implementation, in sync mode, retrying a previously suspended tree is synchronous inside the promise resolution callback. The new implementation schedules an async callback to perform the retry. If multiple promise resolutions occur before the async callback fires, they will be batched together.
Todo
invokeGuardedCallbackReactDOM.createBatch