Passive effect destroy and create functions are interleaved

[bvaughn]

We currently run all passive destroy functions for an individual Fiber before running passive create functions. What we should be doing is running all passive destroy functions for all fibers before running any passive create functions (like we do for layout effects).

The reason this is important is that interleaving destroy/create effects between sibling components might cause components to interfere with each other (e.g. a destroy function in one component may unintentionally override a ref value set by a create function in another component).

We handle this for layout effects by invoking all destroy functions during the "mutation" phase and all create functions during the "layout" phase, but for passive effects we call both in a single traversal:

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

Lines 394 to 409 in 38cd758

	export function commitPassiveHookEffects(finishedWork: Fiber): void {
	if ((finishedWork.effectTag & Passive) !== NoEffect) {
	switch (finishedWork.tag) {
	case FunctionComponent:
	case ForwardRef:
	case SimpleMemoComponent:
	case Chunk: {
	commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
	commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
	break;
	}
	default:
	break;
	}
	}
	}

Fixing this probably means splitting our passive effects loop into two passes:

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

Lines 2179 to 2222 in 38cd758

	function flushPassiveEffectsImpl() {
	if (rootWithPendingPassiveEffects === null) {
	return false;
	}
	const root = rootWithPendingPassiveEffects;
	const expirationTime = pendingPassiveEffectsExpirationTime;
	rootWithPendingPassiveEffects = null;
	pendingPassiveEffectsExpirationTime = NoWork;
	invariant(
	(executionContext & (RenderContext | CommitContext)) === NoContext,
	'Cannot flush passive effects while already rendering.',
	);
	const prevExecutionContext = executionContext;
	executionContext |= CommitContext;
	const prevInteractions = pushInteractions(root);
	// Note: This currently assumes there are no passive effects on the root
	// fiber, because the root is not part of its own effect list. This could
	// change in the future.
	let effect = root.current.firstEffect;
	while (effect !== null) {
	if (__DEV__) {
	setCurrentDebugFiberInDEV(effect);
	invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
	if (hasCaughtError()) {
	invariant(effect !== null, 'Should be working on an effect.');
	const error = clearCaughtError();
	captureCommitPhaseError(effect, error);
	}
	resetCurrentDebugFiberInDEV();
	} else {
	try {
	commitPassiveHookEffects(effect);
	} catch (error) {
	invariant(effect !== null, 'Should be working on an effect.');
	captureCommitPhaseError(effect, error);
	}
	}
	const nextNextEffect = effect.nextEffect;
	// Remove nextEffect pointer to assist GC
	effect.nextEffect = null;
	effect = nextNextEffect;
	}

However this could be a breaking change (since it would affect timing) so we should probably do it behind a feature flag for now.

Also note that splitting this into two passes could have another unintended effect: an error thrown in a passive destroy function would no longer prevent subsequent create functions from being run on that Fiber (as is currently the case) unless we added code to handle that specific case. We should decide what the expected behavior is here.