Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions hooks/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ options._render = vnode => {
hookItem._pendingArgs = hookItem._nextValue = undefined;
});
} else {
hooks._pendingEffects.some(invokeCleanup);
hooks._pendingEffects.some(invokeEffect);
hooks._pendingEffects = [];
if (hooks._pendingEffects.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this the wrong branch of this condition? The fix should be in the branch where currentComponent === previousComponent since it's for the case where the component is dirtied mid-render?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the right branch, when we immediately re-render due to an in-render setState we will reset the components hooks state. When we however have overlapping renders where we see that we have rendered this component before and it still has pending-effects to flush we will flush it. Now instead of flushing the component only we flush the entire subtree.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason for this is that nothing currently stops the following from happening

  • Queue update
  • Update runs, our root and an intermediary component enqueues an effect run
  • Child queues an update for parent
    • BEFORE: we flush the intermediary effect
    • AFTER: we flush both intermediary as well as root effect
  • intermediary renders again

Between 3 and 4 we would flush the parent updates

flushAfterPaintEffects();
}
currentIndex = 0;
}
}
Expand Down
60 changes: 59 additions & 1 deletion hooks/test/browser/useEffect.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { act, teardown as teardownAct } from 'preact/test-utils';
import { createElement, render, Fragment, Component } from 'preact';
import { createElement, render, Fragment, Component, options } from 'preact';
import { useEffect, useState, useRef } from 'preact/hooks';
import { setupScratch, teardown } from '../../../test/_util/helpers';
import { useEffectAssertions } from './useEffectAssertions';
Expand Down Expand Up @@ -665,4 +665,62 @@ describe('useEffect', () => {
setVal(1);
});
});

it('should flush all pending effects when one component re-renders before afterPaint', () => {
// Regression test: when only one component re-renders (via setState)
// before afterPaint has flushed effects, ALL pending effects from
// ALL components should be flushed as a batch before the re-render
// proceeds. This matches React's flushPendingEffects() behavior.
//
// Without the fix, only the re-rendering component's stale effects
// were flushed in _render, leaving sibling effects unflushed until
// they got their own re-render or afterPaint fired.
const order = [];
let setB;

function A() {
useEffect(() => {
order.push('A effect');
}, []);
return <div>A</div>;
}

function B() {
const [val, _setB] = useState(0);
setB = _setB;
useEffect(() => {
order.push('B effect');
}, []);
return <div>{val}</div>;
}

function App() {
return (
<div>
<A />
<B />
</div>
);
}

render(<App />, scratch);

// Effects are queued but not yet flushed (afterPaint hasn't fired).
expect(order).to.deep.equal([]);

// Force B to re-render synchronously by overriding debounceRendering.
// This simulates what happens when a store subscription or
// useLayoutEffect triggers a re-render before afterPaint.
const prev = options.debounceRendering;
options.debounceRendering = cb => cb();
setB(1);
options.debounceRendering = prev;

// With the fix: B's _render detects stale effects and calls
// flushAfterPaintEffects(), which flushes BOTH A's and B's effects.
// Without the fix: only B's effects would flush here; A's effect
// would be deferred.
expect(order).to.include('A effect');
expect(order).to.include('B effect');
});
});
Loading