[Fiber] Various minor tweaks and a few big ones#7248
Merged
sebmarkbage merged 13 commits intofacebook:masterfrom Aug 5, 2016
Merged
[Fiber] Various minor tweaks and a few big ones#7248sebmarkbage merged 13 commits intofacebook:masterfrom
sebmarkbage merged 13 commits intofacebook:masterfrom
Conversation
ghost
added
sebmarkbage
force-pushed
the
fibercleanup
branch
from
aeef0e5 to
c9be6b3
Compare
sebmarkbage
force-pushed
the
fibercleanup
branch
4 times, most recently
from
8416871 to
e3cd993
Compare
sebmarkbage
changed the title
Merged
sebmarkbage
force-pushed
the
fibercleanup
branch
from
a20b326 to
26d1314
Compare
| // this work. We should only do this when we're stepping upwards because | ||
| // completing a downprioritized item is not the same as completing its | ||
| // children. | ||
| if (workInProgress.childInProgress) { |
Contributor
Author
There was a problem hiding this comment.
This let me get rid of the wasDeprioritized flag. We only do work when stepping backup instead of when we're completing a node that was deprioritized (with a "hidden" attribute).
Collaborator
|
Reviewed through "Introduce shouldComponentUpdate in Fiber", will look at the last commit when I have some more time. |
| // pending props from that. We want it to read our priority level and | ||
| // pending props from the work in progress. Needs restructuring. | ||
| return findNextUnitOfWorkAtPriority(workInProgress, priorityLevel); | ||
| var work = findNextUnitOfWorkAtPriority(workInProgress, priorityLevel); |
Collaborator
|
Good enough, accepted. :) |
Add unmounting hook.
Mostly just to get started with unit testing.
The code is sufficiently complex now that this is more noise than helpful. Will just temporary explicit logs in the future.
I'm paranoid about inline-ability so I use this pattern of adding a constant to the closure everywhere. ES6 modules help avoid that but we can't use that consistently because of the dependency injection so instead I opt for making this explicit everywhere. Grep: \b[a-zA-Z_$\d]+\.[a-zA-Z_$\d]+\(
If the current work is higher priority than the new work, then don't bother resetting the unit of work pointer since it won't affect the execution order.
This might become confusing later but is unreachable today. That's because existing pendingProps only matter when a clone is updated, but this path can only matter when it is created.
When we downprioritize children we need to remember to reuse the old children in the update side-effect. This whole set up is very unfortunate since we can have children in our active tree that never actually finished rendering. This strategy might be fundamentally flawed, not sure.
Thanks @acdlite. Add comment about future unit test coverage. This was actually hiding the fact that we are only able to reuse existing work if it was marked as completely finished which it won't be if we reuse its pending priority. However, we should be able to bail out even if there is work remaining in a subtree.
We need to use the *other* child because we reset it to the current one on the way up. We also need to reset the first/last side-effects to that of the children so that we're committing the right thing.
It is important to be able to use this since it avoids starvation problems if you can reuse partially completed work.
This took a while to figure out, but we need to be able to store children that are currently being worked on separately from the current children. We always need a canonical "current" children so that we can update them. However, we also need a different set that we're currently working on so that we have a way to get to already progressed work. This solve the starvation problem in the first render because now we can reach children that were never rendered and have a place to store their progressed work on. The unit test changes tests this. This lets us get rid of the hasWorkInProgress flag. When we reconcile new children we need to reconcile them against progressed work so that we can reuse it. The progressed work is "work in progress" nodes. So in that case we need to mutate instead of clone, to preserve the invariant that only two versions exist at any given point. This effectively forks the ReactChildFiber implementation.
sebmarkbage
force-pushed
the
fibercleanup
branch
from
26d1314 to
af7dd8e
Compare
Contributor
Author
|
Regarding the last commit... It is currently not possible to flush updates to a subtree without also "completing" the parent of that subtree. This means that we can't actually make updates to the Storing the old tree is actually unnecessary right now. The actual purpose of the field is rather that we know whether the child set is already flushed or not already flushed. |
Contributor
Author
|
This is not the same as not needing the two parallel trees though. We can still abort an update which would restore the tree to its previous state. The case I'm talking about above only happens when we've flushed a tree already but parts of it was downprioritized. At that point we can't really safely restore those children since the parent props that lead to them existing in the first place have already been flushed. That begs the question how useful downprioritization is for anything other than hidden/offscreen content. |
zpao
pushed a commit
that referenced
this pull request
Oct 4, 2016
[Fiber] Various minor tweaks and a few big ones (cherry picked from commit a8c8191)
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
See individual commits.