Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix hook update not being applied when a forced context update was enqueued at the same time #3922

Merged
merged 5 commits into from
Mar 8, 2023
Merged
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
30 changes: 27 additions & 3 deletions hooks/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,37 @@ export function useReducer(reducer, initialState, init) {

if (!currentComponent._hasScuFromHooks) {
currentComponent._hasScuFromHooks = true;
const prevScu = currentComponent.shouldComponentUpdate;
let prevScu = currentComponent.shouldComponentUpdate;
const prevCWU = currentComponent.componentWillUpdate;

// If we're dealing with a forced update `shouldComponentUpdate` will
// not be called. But we use that to update the hook values, so we
// need to call it.
currentComponent.componentWillUpdate = function(p, s, c) {
marvinhagemeister marked this conversation as resolved.
Show resolved Hide resolved
if (this._force) {
let tmp = prevScu;
// Clear to avoid other sCU hooks from being called
prevScu = undefined;
marvinhagemeister marked this conversation as resolved.
Show resolved Hide resolved
updateHookState(p, s, c);
prevScu = tmp;
}

if (prevCWU) prevCWU.call(this, p, s, c);
};

// This SCU has the purpose of bailing out after repeated updates
// to stateful hooks.
// we store the next value in _nextValue[0] and keep doing that for all
// state setters, if we have next states and
// all next states within a component end up being equal to their original state
// we are safe to bail out for this specific component.
currentComponent.shouldComponentUpdate = function(p, s, c) {
/**
*
* @type {import('./internal').Component["shouldComponentUpdate"]}
*/
// @ts-ignore - We don't use TS to downtranspile
// eslint-disable-next-line no-inner-declarations
function updateHookState(p, s, c) {
if (!hookState._component.__hooks) return true;

const stateHooks = hookState._component.__hooks._list.filter(
Expand Down Expand Up @@ -223,7 +245,9 @@ export function useReducer(reducer, initialState, init) {
? prevScu.call(this, p, s, c)
: true
: false;
};
}

currentComponent.shouldComponentUpdate = updateHookState;
}
}

Expand Down
58 changes: 56 additions & 2 deletions hooks/test/browser/combinations.test.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { setupRerender, act } from 'preact/test-utils';
import { createElement, render, Component } from 'preact';
import { createElement, render, Component, createContext } from 'preact';
import { setupScratch, teardown } from '../../../test/_util/helpers';
import {
useState,
useReducer,
useEffect,
useLayoutEffect,
useRef,
useMemo
useMemo,
useContext
} from 'preact/hooks';
import { scheduleEffectAssert } from '../_util/useEffectUtil';

Expand Down Expand Up @@ -344,4 +345,57 @@ describe('combinations', () => {

expect(ops).to.deep.equal(['child effect', 'parent effect']);
});

it('should not block hook updates when context updates are enqueued', () => {
const Ctx = createContext({
value: 0,
setValue: /** @type {*} */ () => {}
});

let triggerSubmit = () => {};
function Child() {
const ctx = useContext(Ctx);
const [shouldSubmit, setShouldSubmit] = useState(false);
triggerSubmit = () => setShouldSubmit(true);

useEffect(() => {
if (shouldSubmit) {
// Update parent state and child state at the same time
ctx.setValue(v => v + 1);
setShouldSubmit(false);
}
}, [shouldSubmit]);

return <p>{ctx.value}</p>;
}

function App() {
const [value, setValue] = useState(0);
const ctx = useMemo(() => {
return { value, setValue };
}, [value]);
return (
<Ctx.Provider value={ctx}>
<Child />
</Ctx.Provider>
);
}

act(() => {
render(<App />, scratch);
});

expect(scratch.textContent).to.equal('0');

act(() => {
triggerSubmit();
});
expect(scratch.textContent).to.equal('1');

// This is where the update wasn't applied
act(() => {
triggerSubmit();
});
expect(scratch.textContent).to.equal('2');
});
});