Reconcile Call component children with current

[rmhartog]

Solves the issue occurring in #11955. Because the workInProgress.stateNode was used instead of current.stateNode, some Fibers were being deleted twice, causing an invalid state.

The implementation of updateCallComponent is now also closer to reconcileChildrenAtExpirationTime.

[rmhartog]

I'm also wondering if the second argument of mountChildFibers should be null:

react/packages/react-reconciler/src/ReactFiberBeginWork.js

Lines 552 to 559 in 48833f6

	if (current === null) {
	workInProgress.stateNode = mountChildFibers(
	workInProgress,
	workInProgress.stateNode,
	nextChildren,
	renderExpirationTime,
	);
	} else {

same as in reconcileChildrenAtExpirationTime?

react/packages/react-reconciler/src/ReactFiberBeginWork.js

Lines 118 to 129 in 48833f6

	if (current === null) {
	// If this is a fresh new component that hasn't been rendered yet, we
	// won't update its child set by applying minimal side-effects. Instead,
	// we will add them all to the child before it gets rendered. That means
	// we can optimize this reconciliation pass by not tracking side-effects.
	workInProgress.child = mountChildFibers(
	workInProgress,
	null,
	nextChildren,
	renderExpirationTime,
	);
	} else {

If so, perhaps it's possible to swap child and stateNode, call reconcileChildrenAtExpirationTime and swap them back, to avoid the 'fork' from getting out of sync in the future. It may hurt readability though, so I'm not too sure about that idea.

Perhaps @gaearon or @sebmarkbage could comment on that?

[gaearon]

Nice, mind adding a test that would previously throw but now passes?

[rmhartog]

Will do!

[gaearon]

Seeing that you just dug into the reconciler code for the first time, how was your experience? What do you wish was more documented, what was most confusing etc.

[rmhartog]

Added a test for un- and remounting behaviour.

I limited myself to the parts of the reconciler that related to the calls and returns. Once I understood the relevant fields and structure of Fiber, and added some log statements throughout, the process started to fall into place.

What was most unclear to me is the status of some of the TODO's I ran into, perhaps linking to an issue which contains more context could be useful, such as:

react/packages/react-reconciler/src/ReactFiber.js

Lines 360 to 367 in 65aeb70

	// Currently assumed to be a continuation and therefore is a
	// fiber already.
	// TODO: The yield system is currently broken for updates in some
	// cases. The reified yield stores a fiber, but we don't know which
	// fiber that is; the current or a workInProgress? When the
	// continuation gets rendered here we don't know if we can reuse that
	// fiber or if we need to clone it. There is probably a clever way to
	// restructure this.

react/packages/react-reconciler/src/ReactFiberBeginWork.js

Lines 543 to 545 in 65aeb70

	// TODO: When bailing out, we might need to return the stateNode instead
	// of the child. To check it for work.
	// return bailoutOnAlreadyFinishedWork(current, workInProgress);

[gaearon]

Hmm good finds. I think some (both?) of these might be outdated and no longer apply.