[Fiber] Add more life-cycles #8015
Conversation
sebmarkbage
force-pushed
the
fiberclassinit
branch
2 times, most recently
from
a5d2c74
to
2969381
Compare
sebmarkbage
added
GH Review: needs-revision
and removed
GH Review: review-needed
labels
|
This infinite loops on a test (and probably needs some new unit tests to get coverage in the new scenarios). EDIT: I need to fix the scenario when new roots are rendered or other components gets |
|
|
||
| // Note: During these life-cycles, instance.props/instance.state are what | ||
| // ever the previously attempted to render - not the "current". However, | ||
| // during componentDidUpdate we pass the "current" props. |
There was a problem hiding this comment.
This is the intended behavior? I wonder if this breaks any user assumptions.
There was a problem hiding this comment.
Like discussed in chat, this is intended. It's a bit confusing but it's the most likely semantics that will work with current code.
The primary goal is to have a nicer upgrade path from existing code. Ease of understanding the semantics of the life-cycles can be achieved by redesigning a new API for them.
sebmarkbage
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
sebmarkbage
force-pushed
the
fiberclassinit
branch
3 times, most recently
from
d82179f
to
7136730
Compare
|
Accepting to unblock because I haven't been able to review yet. |
This refactors the initialization process so that we can share it with the "module pattern" initialization. There are a few new interesting scenarios unlocked by this. E.g. constructor -> componentWillMount -> shouldComponentUpdate -> componentDidMount when a render is aborted and resumed. If shouldComponentUpdate returns true then we create a new instance instead of trying componentWillMount again or componentWillReceiveProps without being mounted. Another strange thing is that the "previous props and state" during componentWillReceiveProps, shouldComponentUpdate and componentWillUpdate are all the previous render attempt. However, componentDidMount's previous is the props/state at the previous commit. That is because the first three can execute multiple times before a didMount.
We currently only filter out "NoWork" in the beginning of this function. If the NoWork root is after the one with work it will show up in the second loop. There's probably a more efficient way of doing this but this works for now. This showed up in this PR because a new unit test gets unblocked which ends up with this case.
If work has progressed on a state update that gets resumed because of another state up, then we won't have an new pendingProps, and we also won't have any memoizedProps because it got aborted before completing. In that case, we can just fallback to the current props. I think that they can't have diverged because the only way they diverge is if there is new props. This lets us bail out on state only updates in more cases which the unit tests reflect.
This tests the life-cycles when work gets aborted.
sebmarkbage
force-pushed
the
fiberclassinit
branch
from
7136730
to
37ca387
Compare
|
|
||
| // A class component update is the result of either new props or new state. | ||
| // Account for the possibly of missing pending props by falling back to the | ||
| // memoized props. |
There was a problem hiding this comment.
What is this comment referring to? You throw on missing pendingProps below. Copypaste?
There was a problem hiding this comment.
yea. This is copypaste.
| !(updateQueue && updateQueue.isForced) && | ||
| workInProgress.memoizedProps !== null && | ||
| !instance.shouldComponentUpdate(newProps, newState)) { | ||
| // TODO: Should this get the new props/state updated regardless? |
There was a problem hiding this comment.
Do you mean whether we should reassign this.props and this.state despite sCU returning false? Is there any reason not to?
|
Read through it. Seems about as sensible as quantum mechanics. |
Builds on #8010
This refactors the initialization process so that we can share it with the "module pattern" initialization.
There are a few new interesting scenarios unlocked by this. E.g. constructor -> componentWillMount -> shouldComponentUpdate -> componentDidMount when a render is aborted and resumed. If shouldComponentUpdate returns true then we create a new instance instead of trying componentWillMount again or componentWillReceiveProps without being mounted.
Another strange thing is that the "previous props and state" (instance props) during componentWillReceiveProps, shouldComponentUpdate and componentWillUpdate are all the props/state at previous render attempt. However, componentDidMount's previous is the props/state at the previous commit. That is because the first three can execute multiple times before a didMount.