-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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
[Fiber] Some setState related issues #8010
Conversation
// If there isn't any new props, then we'll reuse the memoized props. | ||
// This could be from already completed work. | ||
props = workInProgress.memoizedProps; | ||
// props = current.memoizedProps; |
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.
Dead code or intentional comment?
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.
oops. left over.
We don't want to use the "current" props/state because if we have started work, then pause and continue then we'll have the newer props/state on it already. If it is not a resume, it should be the same as current.
Currently we flag the path to a setState as a higher priority than "offscreen". When we reconcile down this tree we bail out if it is a hidden node. However, in the case that node is already completed we don't hit that bail out path. We end up doing the work immediately which ends up committing that part of the tree at a higher priority. This ensures that we don't let a deprioritized subtree get reconciled at a higher priority.
6a862e7
to
cb2bc0d
Compare
// Still not updated. | ||
span(0), | ||
span(0), | ||
span(0) |
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.
How do know these spans haven't updated? idx
hasn't changed, so even if you flush completely on line 654 you would still expect these to be 0. (On that point, what is the purpose of idx
in this test?)
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.
Fixed. 23857e8
This proves that this number is actually deprioritized.
Looks good! Took me a while to figure out that second commit, though :D |
It took me almost a week to find a repro when I found the bug in the demo. :) |
* Use the memoized props/state from the workInProgress We don't want to use the "current" props/state because if we have started work, then pause and continue then we'll have the newer props/state on it already. If it is not a resume, it should be the same as current. * Deprioritize setState within a deprioritized tree Currently we flag the path to a setState as a higher priority than "offscreen". When we reconcile down this tree we bail out if it is a hidden node. However, in the case that node is already completed we don't hit that bail out path. We end up doing the work immediately which ends up committing that part of the tree at a higher priority. This ensures that we don't let a deprioritized subtree get reconciled at a higher priority. * Bump idx in unit test This proves that this number is actually deprioritized.
Use the memoized props/state from the workInProgress
We don't want to use the "current" props/state because if we have started work, then pause and continue then we'll have the newer props/state on it already. If it is not a resume, it should be the same as current.
Deprioritize setState within a deprioritized tree
Currently we flag the path to a setState as a higher priority than "offscreen". When we reconcile down this tree we bail out if it is a hidden node. However, in the case that node is already completed we don't hit that bail out path. We end up doing the work immediately which ends up committing that part of the tree at a higher priority.
This ensures that we don't let a deprioritized subtree get reconciled at a higher priority.