Unconditionally flushing effects is unsafe

[billyjanitsch]

Hi! This implementation has a bug that can cause effects to be permanently skipped.

The special flush event clears all pending effects from the queue:

useEffectReducer/src/index.tsx

Lines 72 to 75 in 17da4f9

	if (event === flushEffectsSymbol) {
	// Record that effects have already been executed
	return [state, []];
	}

This seems fine because event is dispatched after clearing all effects in the queue:

useEffectReducer/src/index.tsx

Lines 92 to 94 in 17da4f9

	useEffect(() => {
	if (stateEffectTuples.length) {
	stateEffectTuples.forEach(([stateForEffect, effects]) => {

useEffectReducer/src/index.tsx

Line 116 in 17da4f9

dispatch(flushEffectsSymbol);

The problem is that this effect closes over stateEffectTuples. It's possible that new effects will be added to the queue in between the render and the execution of the effect. These effects won't be executed in the effect because of the closure, but they will be cleared by the flush event. So they will effectively be skipped.

Here's a sandbox demonstrating the problem. Click "increment" and notice that the state correctly updates to 2, but the console has only printed INC 0. We'd expect it to also print INC 1. The latter effect was skipped because the second increment was added to the queue after the closure but before the dispatch of the flush event. So the effect gets added to the queue and then cleared without having a chance to run.

One potential solution would be to slice off the first stateEffectTuples.length elements from the state at the time of the update rather than unconditionally returning []. Essentially this is saying "flush the effects that were just run" rather than "flush all effects".

[davidkpiano]

Can you make a failing test for this issue? Or better yet, a PR?

Otherwise I'll get to this soon.