Skip to content

Commit

Permalink
Allow handling errors in getSnapshot of useSyncExternalStore & add mo…
Browse files Browse the repository at this point in the history 
…re tests (#4175)

* Move useSyncExternalStore tests to separate file

* Add React tests for useSyncExternalStore

* Remove irrelevant React tests

* Add test for store mutations before subscribing
  • Loading branch information
@andrewiggins
andrewiggins authored Oct 30, 2023
1 parent 9956916 commit 62c050f
Show file tree
Hide file tree
Showing 3 changed files with 830 additions and 179 deletions.
23 changes: 20 additions & 3 deletions compat/src/index.js
View file
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
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

0 comments on commit 62c050f

Please sign in to comment.