[Fiber] "Task" priority for error boundaries and batched updates #8193
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
There was lots of duplication across all the scheduling functions. I think we're far enough along that we can start trying to clean some stuff up. Also introduces a new priority level provisionally called Task priority. This is for work that completes at the end of the current tick, after the current batch of work has been committed. It's different from Synchronous priority, which needs to complete immediately. A full implementation of Task priority will follow. It will replace the current batching solution.
Task priority is similar to Synchronous priority. Both are flushed in the current tick. Synchronous priority is flushed immediately (e.g. sync work triggered by setState will flush before setState exits), where as Task is flushed after the current batch of work is committed. Currently used for batchedUpdates and nested sync updates. Task should also be used for componentDidUpdate/Mount and error boundary work. I'll add this in a later commit.
c1e5d8d to
25d9db5
Compare
I have all but one error fixed. Not sure how tricky the last one is, but I'm cautiously optimistic. Pushing to show my current progress.
9611e06 to
2cc85e6
Compare
|
Pushed a new commit that passes all the error boundary tests. Changed the algorithm of This passes all the error boundary tests and the incremental tests; however, it's causing an infinite loop in one of the files when I try to run the full suite. Haven't figured out which one yet. Unfortunately, the infinite loop means I can't run the I feel pretty good about the overall approach here, though. Will continue to investigate which test is breaking tonight; if I can't find it, I'll pick it up in the morning. |
334bcbb to
ea57b68
Compare
|
Found it! The test is |
ea57b68 to
90d07ba
Compare
|
Yay! I got it working! Could use a bit more cleanup, but this should be ready for review @gaearon. |
acdlite
commented
Nov 4, 2016
| if (maybeErrorBoundary.tag === ClassComponent) { | ||
| const instance = maybeErrorBoundary.stateNode; | ||
| if (typeof instance.unstable_handleError === 'function' && | ||
| !knownBoundaries.has(maybeErrorBoundary)) { |
There was a problem hiding this comment.
This line here is how I got it to work, and why I had to move this function into the scheduler.
6ddb725 to
c3cad3b
Compare
Changed the algorithm of handleErrors a bit to ensure that boundaries are not revisited once they are acknowledged.
c3cad3b to
b407b9a
Compare
Discovered an edge case: a noop error boundary will break in incremental mode unless you explicitly schedule an update.
[Fiber] Refactor error boundaries in Fiber
The existing logic was written before we had a proper error handling system.
gaearon
approved these changes
Nov 4, 2016
gaearon
reviewed
Nov 4, 2016
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| var ReactDOMFeatureFlags = { | |||
| useCreateElement: true, | |||
| useFiber: false, | |||
| useFiber: true, | |||
There was a problem hiding this comment.
Unless you consider it ready for 15.4.0 😄
|
|
acusti
pushed a commit
to brandcast/react
that referenced
this pull request
Mar 15, 2017
…ebook#8193) * Refactor scheduling functions and introduce Task priority There was lots of duplication across all the scheduling functions. I think we're far enough along that we can start trying to clean some stuff up. Also introduces a new priority level provisionally called Task priority. This is for work that completes at the end of the current tick, after the current batch of work has been committed. It's different from Synchronous priority, which needs to complete immediately. A full implementation of Task priority will follow. It will replace the current batching solution. * Implement Task priority Task priority is similar to Synchronous priority. Both are flushed in the current tick. Synchronous priority is flushed immediately (e.g. sync work triggered by setState will flush before setState exits), where as Task is flushed after the current batch of work is committed. Currently used for batchedUpdates and nested sync updates. Task should also be used for componentDidUpdate/Mount and error boundary work. I'll add this in a later commit. * Make error boundaries use Task Priority I have all but one error fixed. Not sure how tricky the last one is, but I'm cautiously optimistic. Pushing to show my current progress. * Remove recursion from handleErrors Changed the algorithm of handleErrors a bit to ensure that boundaries are not revisited once they are acknowledged. * Add incremental error boundary test Discovered an edge case: a noop error boundary will break in incremental mode unless you explicitly schedule an update. * Refactor error boundaries in Fiber * Simplify trapError() calls The existing logic was written before we had a proper error handling system. * Remove unnecessary flags * Prevent performTaskWork recursion * Be stricter about preventing recursion
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.
Introduces a new priority level, Task. (The name is provisional. We'll come up with something better later.)
Task priority is similar to Synchronous priority. Both are flushed in the current tick. The difference is
setStatewill perform all its work before thesetStatecall exits.In #8127, these two types of work priority were both considered "synchronous." Splitting them into separate concepts will make it easier to schedule and batch things correctly.
Currently used for
batchedUpdatesand nested sync updates. Task should also be used for updates insidecomponentDidUpdate/Mount, error boundary work, and events. I'll leave these for a future PR.There's an error boundary regression that I haven't looked at yet. I'll investigate it in the morning. Pushing this now because I'm about to go to bed and I don't want to block @gaearon.