[Fiber] Remove output field #8406
Conversation
sebmarkbage
force-pushed
the
fiberremoveoutput
branch
from
ad5c528 to
1b60d73
Compare
| nextChildren, | ||
| priorityLevel | ||
| ); | ||
| } else if (current.child === workInProgress.child) { |
There was a problem hiding this comment.
Shouldn't this be current.stateNode === workInProgress.stateNode?
I changed it, and tests still pass.
Is this unobservable, or are we missing a test?
There was a problem hiding this comment.
We no coverage of time slicing or bailouts for coroutines at all yet.
|
|
||
| transferDeletions(workInProgress); | ||
| } | ||
| // markChildAsProgressed(current, workInProgress, priorityLevel); |
There was a problem hiding this comment.
I get that we can't do this now because there is no equivalent of the progressedChild for the stateNode. Does this affect correctness, or does it just mean coroutines aren't as capable of reusing work? Is this comment actionable or can it be removed?
There was a problem hiding this comment.
This is a bit of a WIP still. It might be correctness related. I'm not even sure.
sebmarkbage
force-pushed
the
fiberremoveoutput
branch
from
1b60d73 to
05960df
Compare
| @@ -296,7 +296,41 @@ module.exports = function<T, P, I, TI, C>( | |||
| if (!coroutine) { | |||
| throw new Error('Should be resolved by now'); | |||
| } | |||
| reconcileChildren(current, workInProgress, coroutine.children); | |||
|
|
|||
| const nextChildren = coroutine.children; | |||
There was a problem hiding this comment.
I'd appreciate a comment here mentioning that this code is pretty much the same as reconcileChildren(), but operating on stateNode.
| @@ -288,7 +288,7 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) { | |||
|
|
|||
| const returnFiber = workInProgress.return; | |||
|
|
|||
| if (returnFiber) { | |||
| if (returnFiber && !next) { | |||
There was a problem hiding this comment.
Why does it hang without this change?
There was a problem hiding this comment.
Because the first time the coroutine completes, it adds itself to the effect list. Then it does its child, and completes again.
Not sure what happens if there are no children in the first phase. It probably breaks anyway.
There was a problem hiding this comment.
Actually, it's because the new test uses classes which have "update effects" scheduled on them to invoke the life-cycles. Those effects gets scheduled twice.
|
I'll land just the first two commits. |
The idea was originally that each fiber has a return value. In practice most of what we're modelling here are void functions and we track side effects instead of return values. We do use this for coroutines to short cut access to terminal yields. However, since this can be nested fragments we end up searching the tree anyway. We also have to manage this in transferOutput so it ends up being as expensive. Maybe we can save some traversal for updates when coroutine branches bail out but meh.
sebmarkbage
force-pushed
the
fiberremoveoutput
branch
from
05960df to
4910b16
Compare
`output` filed has been removed. facebook/react#8406
* Remove output field The idea was originally that each fiber has a return value. In practice most of what we're modelling here are void functions and we track side effects instead of return values. We do use this for coroutines to short cut access to terminal yields. However, since this can be nested fragments we end up searching the tree anyway. We also have to manage this in transferOutput so it ends up being as expensive. Maybe we can save some traversal for updates when coroutine branches bail out but meh. * Unmount child from the first phase of a coroutine
The idea was originally that each fiber has a return value. In practice most of what we're modeling here are void functions and we track side effects instead of return values.
We do use this for coroutines to short cut access to terminal yields. However, since this can be nested fragments we end up searching the tree anyway. We also have to manage this in transferOutput so it ends up being as expensive. Maybe we can save some traversal for updates when coroutine branches bail out but meh.
Swapped the
.childand.stateNodeon coroutines so that.stateNodeis the first phase and.childis the completed phase. This is a bit more useful.Unmount nodes from the first phase when the parent is unmounted.