From 9e9df924e33581869fcac67147ccac4eb12d294c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 1 Sep 2021 18:14:26 -0400 Subject: [PATCH] [wip] Combine useRef and useState hooks Since the useState hook is only used to force a render, and we don't use it to track any state, we can stash our mutable instance object in there instead of using a separate useRef hook. Doesn't affect any behavior, just saves a bit of hook memory. --- .../useSyncExternalStoreShared-test.js | 33 ------------- .../src/useSyncExternalStore.js | 46 +++++++------------ 2 files changed, 17 insertions(+), 62 deletions(-) diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js index d919f35b1e0a9f..8db73a8e8c77c3 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -96,9 +96,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }; } - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('basic usage', () => { const store = createExternalStore('Initial'); @@ -141,9 +138,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('Initial'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('switch to a different store', () => { const storeA = createExternalStore(0); const storeB = createExternalStore(0); @@ -193,9 +187,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('1'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('selecting a specific value inside getSnapshot', () => { const store = createExternalStore({a: 0, b: 0}); @@ -239,9 +230,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A1B1'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test( "compares to current state before bailing out, even when there's a " + 'mutation in between the sync and passive effects', @@ -284,9 +272,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }, ); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('mutating the store in between render and commit when getSnapshot has changed', () => { const store = createExternalStore({a: 1, b: 1}); @@ -345,9 +330,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('B2'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('mutating the store in between render and commit when getSnapshot has _not_ changed', () => { // Same as previous test, but `getSnapshot` does not change const store = createExternalStore({a: 1, b: 1}); @@ -405,9 +387,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A1'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test("does not bail out if the previous update hasn't finished yet", () => { const store = createExternalStore(0); @@ -443,9 +422,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('00'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('uses the latest getSnapshot, even if it changed in the same batch as a store update', () => { const store = createExternalStore({a: 0, b: 0}); @@ -473,9 +449,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('2'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('handles errors thrown by getSnapshot or isEqual', () => { class ErrorBoundary extends React.Component { state = {error: null}; @@ -552,9 +525,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }); describe('extra features implemented in user-space', () => { - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('memoized selectors are only called once per update', () => { const store = createExternalStore({a: 0, b: 0}); @@ -593,9 +563,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(root).toMatchRenderedOutput('A1'); }); - // 18 has an experimental flag to warn about reading refs. Will circumvent - // when built-in API is implemented. - // @gate !enableUseRefAccessWarning || !__DEV__ test('Using isEqual to bailout', () => { const store = createExternalStore({a: 0, b: 0}); diff --git a/packages/use-sync-external-store/src/useSyncExternalStore.js b/packages/use-sync-external-store/src/useSyncExternalStore.js index 0bc8b0d2390a75..1b49fe3c1be717 100644 --- a/packages/use-sync-external-store/src/useSyncExternalStore.js +++ b/packages/use-sync-external-store/src/useSyncExternalStore.js @@ -14,7 +14,6 @@ import is from 'shared/objectIs'; // dispatch for CommonJS interop named imports. const { useState, - useRef, useEffect, useLayoutEffect, useDebugValue, @@ -59,34 +58,27 @@ function useSyncExternalStore_shim( } } - // Because updates are synchronous, we don't queue them. Instead we force a - // re-render whenever the subscribed state changes, and read the current state - // directly in render. - // - // We force a re-render by bumping a version number. - const [, setVersion] = useState(0); - // Read the current snapshot from the store on every render. Again, this // breaks the rules of React, and only works here because of specific // implementation details, most importantly that updates are // always synchronous. const value = getSnapshot(); + // Because updates are synchronous, we don't queue them. Instead we force a + // re-render whenever the subscribed state changes by updating an some + // arbitrary useState hook. Then, during render, we call getSnapshot to read + // the current value. + // + // Because we don't actually use the state returned by the useState hook, we + // can save a bit of memory by storing other stuff in that slot. + // // To implement the early bailout, we need to track some things on a mutable - // ref object. Initialize this object on the first render. - const refs = useRef(null); - let inst; - if (refs.current === null) { - inst = { - // This represents the currently rendered value and getSnapshot function. - // We update them with a ref whenever they change. - value, - getSnapshot, - }; - refs.current = inst; - } else { - inst = refs.current; - } + // object. Usually, we would put that with a useRef hook, but we can stash + // that in our useState hook instead. + // + // To force a re-render, we call forceUpdate({inst}). That works because the + // new object always fails an equality check. + const [{inst}, forceUpdate] = useState({inst: {value, getSnapshot}}); // Track the latest getSnapshot function with a ref. This needs to be updated // in the layout phase so that we can access it during the tearing check that @@ -102,7 +94,7 @@ function useSyncExternalStore_shim( // effect may have mutated the store. if (checkIfSnapshotChanged(inst)) { // Force a re-render. - setVersion(bumpVersion); + forceUpdate({inst}); } }, [subscribe, value, getSnapshot]); @@ -111,7 +103,7 @@ function useSyncExternalStore_shim( // detected in the subscription handler. if (checkIfSnapshotChanged(inst)) { // Force a re-render. - setVersion(bumpVersion); + forceUpdate({inst}); } const handleStoreChange = () => { // TODO: Because there is no cross-renderer API for batching updates, it's @@ -123,7 +115,7 @@ function useSyncExternalStore_shim( // read from the store. if (checkIfSnapshotChanged(inst)) { // Force a re-render. - setVersion(bumpVersion); + forceUpdate({inst}); } }; // Subscribe to the store and return a clean-up function. @@ -144,7 +136,3 @@ function checkIfSnapshotChanged(inst) { return true; } } - -function bumpVersion(v) { - return v + 1; -}