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.

Sign up for GitHub

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

Allow handling errors in getSnapshot of useSyncExternalStore & add more tests #4175

Merged
merged 4 commits into from
Oct 30, 2023
Merged

Allow handling errors in getSnapshot of useSyncExternalStore & add more tests #4175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89013f4
Move useSyncExternalStore tests to separate file
andrewiggins Oct 27, 2023
4b25f93
Add React tests for useSyncExternalStore
andrewiggins Oct 27, 2023
44c07c0
Remove irrelevant React tests
andrewiggins Oct 27, 2023
a1ce2a7
Add test for store mutations before subscribing
andrewiggins Oct 27, 2023
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
23 changes: 20 additions & 3 deletions compat/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,15 @@ export const isElement = isValidElement;
/**
* This is taken from https://github.com/facebook/react/blob/main/packages/use-sync-external-store/src/useSyncExternalStoreShimClient.js#L84
* on a high level this cuts out the warnings, ... and attempts a smaller implementation
* @typedef {{ _value: any; _getSnapshot: () => any }} Store
*/
export function useSyncExternalStore(subscribe, getSnapshot) {
const value = getSnapshot();

/**
* @typedef {{ _instance: Store }} StoreRef
* @type {[StoreRef, (store: StoreRef) => void]}
*/
const [{ _instance }, forceUpdate] = useState({
_instance: { _value: value, _getSnapshot: getSnapshot }
});
Expand All @@ -162,18 +167,18 @@ export function useSyncExternalStore(subscribe, getSnapshot) {
_instance._value = value;
_instance._getSnapshot = getSnapshot;

if (!is(_instance._value, getSnapshot())) {
if (didSnapshotChange(_instance)) {
forceUpdate({ _instance });
}
}, [subscribe, value, getSnapshot]);

useEffect(() => {
if (!is(_instance._value, _instance._getSnapshot())) {
if (didSnapshotChange(_instance)) {
forceUpdate({ _instance });
}

return subscribe(() => {
if (!is(_instance._value, _instance._getSnapshot())) {
if (didSnapshotChange(_instance)) {
forceUpdate({ _instance });
}
});
Expand All @@ -182,6 +187,18 @@ export function useSyncExternalStore(subscribe, getSnapshot) {
return value;
}

/** @type {(inst: Store) => boolean} */
function didSnapshotChange(inst) {
const latestGetSnapshot = inst._getSnapshot;
const prevValue = inst._value;
try {
const nextValue = latestGetSnapshot();
return !is(prevValue, nextValue);
} catch (error) {
return true;
}
}

export * from 'preact/hooks';
export {
version,
Expand Down
176 changes: 0 additions & 176 deletions compat/test/browser/hooks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,9 @@ import React, {
createElement,
useDeferredValue,
useInsertionEffect,
useSyncExternalStore,
useTransition,
render,
useState,
useCallback,
useContext,
useEffect
} from 'preact/compat';
Expand Down Expand Up @@ -81,180 +79,6 @@ describe('React-18-hooks', () => {
});
});

describe('useSyncExternalStore', () => {
it('subscribes and follows effects', () => {
const subscribe = sinon.spy(() => () => {});
const getSnapshot = sinon.spy(() => 'hello world');

const App = () => {
const value = useSyncExternalStore(subscribe, getSnapshot);
return <p>{value}</p>;
};

act(() => {
render(<App />, scratch);
});
expect(scratch.innerHTML).to.equal('<p>hello world</p>');
expect(subscribe).to.be.calledOnce;
expect(getSnapshot).to.be.calledThrice;
});

it('subscribes and rerenders when called', () => {
let flush;
const subscribe = sinon.spy(cb => {
flush = cb;
return () => {};
});
let called = false;
const getSnapshot = sinon.spy(() => {
if (called) {
return 'hello new world';
}

return 'hello world';
});

const App = () => {
const value = useSyncExternalStore(subscribe, getSnapshot);
return <p>{value}</p>;
};

act(() => {
render(<App />, scratch);
});
expect(scratch.innerHTML).to.equal('<p>hello world</p>');
expect(subscribe).to.be.calledOnce;
expect(getSnapshot).to.be.calledThrice;

called = true;
flush();
rerender();

expect(scratch.innerHTML).to.equal('<p>hello new world</p>');
});

it('getSnapshot can return NaN without causing infinite loop', () => {
let flush;
const subscribe = sinon.spy(cb => {
flush = cb;
return () => {};
});
let called = false;
const getSnapshot = sinon.spy(() => {
if (called) {
return NaN;
}

return 1;
});

const App = () => {
const value = useSyncExternalStore(subscribe, getSnapshot);
return <p>{value}</p>;
};

act(() => {
render(<App />, scratch);
});
expect(scratch.innerHTML).to.equal('<p>1</p>');
expect(subscribe).to.be.calledOnce;
expect(getSnapshot).to.be.calledThrice;

called = true;
flush();
rerender();

expect(scratch.innerHTML).to.equal('<p>NaN</p>');
});

it('should not call function values on subscription', () => {
let flush;
const subscribe = sinon.spy(cb => {
flush = cb;
return () => {};
});

const func = () => 'value: ' + i++;

let i = 0;
const getSnapshot = sinon.spy(() => {
return func;
});

const App = () => {
const value = useSyncExternalStore(subscribe, getSnapshot);
return <p>{value()}</p>;
};

act(() => {
render(<App />, scratch);
});
expect(scratch.innerHTML).to.equal('<p>value: 0</p>');
expect(subscribe).to.be.calledOnce;
expect(getSnapshot).to.be.calledThrice;

flush();
rerender();

expect(scratch.innerHTML).to.equal('<p>value: 0</p>');
});

it('should work with changing getSnapshot', () => {
let flush;
const subscribe = sinon.spy(cb => {
flush = cb;
return () => {};
});

let i = 0;
const App = () => {
const value = useSyncExternalStore(subscribe, () => {
return i;
});
return <p>value: {value}</p>;
};

act(() => {
render(<App />, scratch);
});
expect(scratch.innerHTML).to.equal('<p>value: 0</p>');
expect(subscribe).to.be.calledOnce;

i++;
flush();
rerender();

expect(scratch.innerHTML).to.equal('<p>value: 1</p>');
});

it('works with useCallback', () => {
let toggle;
const App = () => {
const [state, setState] = useState(true);
toggle = setState.bind(this, () => false);

const value = useSyncExternalStore(
useCallback(() => {
return () => {};
}, [state]),
() => (state ? 'yep' : 'nope')
);

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

act(() => {
render(<App />, scratch);
});
expect(scratch.innerHTML).to.equal('<p>yep</p>');

toggle();
rerender();

expect(scratch.innerHTML).to.equal('<p>nope</p>');
});
});

it('should release ._force on context-consumers', () => {
let sequence, setSubmitting;
const Ctx = createContext({
Expand Down
Loading