-
Notifications
You must be signed in to change notification settings - Fork 46.3k
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 update priority tracking from Scheduler package #19121
Decouple update priority tracking from Scheduler package #19121
Conversation
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 4294152:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have way too many execution contexts. Scheduler, execution context, suspense config and now this. I think we need to take a step back and come up with a plan for how we get rid of the overhead. Like the existing runWithPriority with a new closure just to set one state is completely unacceptable overhead, so we shouldn't follow that lead.
I understand that this is just one step along refactoring but I worry that we're refactoring to has too much inherent complexity.
Do we have a plan for what we want to do with 1) execution context, 2) runWithPriority and 3) how we can avoid special cases like priorityLevel < UserBlockingPriority
all over the code base?
(this is more of a comment to @acdlite ofc :) )
For perspective the way I look at setState is that in an optimal reactive system it's essentially this:
The goal is to get close to this, because if we're not close then the system doesn't pay for itself, but we're very far away from this for a simple state change atm. I think we might need to do something more radical. |
This PR is meant to address 2 and 3. It doesn't in the current state but that's one of the goals. |
I think we agree on the final end state. The goals for this first step are a bit confusing and contradictory so I'll list them here:
Once the decoupling is complete, then we can move on to actually removing the extra wrappers. This will require some semantic changes and we'll need to audit www for existing uses. |
This is a really great first React PR! |
Sorry, I guess I mean first reconciler PR. I know it's not your first React PR ever :D |
3cace0b
to
0036347
Compare
Details of bundled changes.Comparing: c3e42a9...4294152 react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: c3e42a9...4294152 react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
Outdated
Show resolved
Hide resolved
Published for review, this should be good to go now 👍 |
Summary
We'd like to remove a dependency on the Scheduler priority from React internals by replacing it with a currentLanePriority which will be tracked inside React. This PR starts currentLanePriority implementation by setting it everywhere that the scheduler priority is set, and then logging mismatches as they occur.
The next step is to investigate mismatches and handle them appropriately.