Fiber: Fix reentrant mounting in synchronous mode #8623
Conversation
sophiebits
force-pushed
the
reentrant-mount
branch
from
884df62 to
25d87f8
Compare
sophiebits
changed the title
| @@ -141,13 +134,20 @@ module.exports = function<T, P, I, TI, C, CX>(config : HostConfig<T, P, I, TI, C | |||
| const root : FiberRoot = (container.stateNode : any); | |||
| const current = root.current; | |||
|
|
|||
| root.pendingContext = getContextForSubtree(parentComponent); | |||
| const context = getContextForSubtree(parentComponent); | |||
| if (root.context === null) { | |||
There was a problem hiding this comment.
This seems gross. If I always set pendingContext instead, this test fails, because hasContextChanged() returns true and we rerender Middle despite element equality:
This is because the root hasn't completed when we do the update and so .pendingContext hasn't been moved to .context yet. (?) I think I'm missing something here.
There was a problem hiding this comment.
@acdlite Keeping pendingContext on the root fiber seems fragile and doesn't work correctly with incremental. Can we move it to the update queue of the host root now?
On the other hand, this is all just a temporary hack until we can switch to portals.
There was a problem hiding this comment.
Yeah putting it on the updateQueue seems reasonable.
There was a problem hiding this comment.
I can't figure out how to do this. Will let you do it if you want.
|
|
||
| scheduleTopLevelUpdate(current, element, callback); | ||
|
|
||
| if (__DEV__) { | ||
| if (ReactFiberInstrumentation.debugTool) { | ||
| if (element === null) { | ||
| if (current.type === null) { |
There was a problem hiding this comment.
Is this right? Not sure on this.
There was a problem hiding this comment.
This is always true. If you wanted to check if this was a HostRoot, you'd check the tag, but even if you did this would always be true.
Instead you could probably check if there is an .alternate before calling scheduleTopLevelUpdate. That's usually how we determine if something is initial mount or not.
There was a problem hiding this comment.
It begs the question though... What if element === null and it was the initial mount? E.g. if you unmount in componentDidMount. I suppose we pretend it never happened?
There was a problem hiding this comment.
What if you do a reentrant update from within a synchronous mount?
There was a problem hiding this comment.
How about current.child === null?
I will also move this above the scheduleTopLevelUpdate because that sounds closer to how it works with non-synchronous priority and if we don't then onUpdateContainer is called before onMountContainer due to the reentrancy (the update is called from the mount and finishes before the mount does)
There was a problem hiding this comment.
Oh, the alternate works as you suggested.
|
Ok. Interesting one. Will look tomorrow but seems like @acdlite might have some insight. |
sophiebits
force-pushed
the
reentrant-mount
branch
from
25d87f8 to
41169a2
Compare
We have to set the container in a way that it can be retrieved before we run any user code that could start a new top-level renderer. This API feels about as good to me.