From 6d4bec8bbd12a8b9f4aff3f351f2e696438049e1 Mon Sep 17 00:00:00 2001 From: costrings-frontend-dev Date: Fri, 10 Jan 2025 22:15:20 +0530 Subject: [PATCH 1/6] perf(hooks): Optimize useSyncExternalStoreWithSelector selector calls Add intelligent selector execution tracking to useSyncExternalStoreWithSelector to avoid unnecessary re-renders and selector calls. The hook now tracks which properties are accessed during selection and only re-runs the selector when those specific properties change. - Add Proxy-based property access tracking - Track accessed properties to determine relevant state changes - Skip selector execution when only irrelevant state changes - Add tests verifying selector optimization behavior - Preserve existing memoization and isEqual behavior This improves performance by reducing unnecessary selector executions when unrelated parts of the state change. --- .../useSyncExternalStoreWithSelector-test.js | 217 ++++++++++++++++++ .../src/useSyncExternalStoreWithSelector.js | 100 ++++++-- 2 files changed, 300 insertions(+), 17 deletions(-) create mode 100644 packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js new file mode 100644 index 0000000000000..65fc8c4c959b4 --- /dev/null +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js @@ -0,0 +1,217 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +'use strict'; + +let useSyncExternalStoreWithSelector; +let React; +let ReactDOM; +let ReactDOMClient; +let ReactFeatureFlags; +let act; + +describe('useSyncExternalStoreWithSelector', () => { + beforeEach(() => { + jest.resetModules(); + + if (gate(flags => flags.enableUseSyncExternalStoreShim)) { + // Remove useSyncExternalStore from the React imports so that we use the + // shim instead. Also removing startTransition, since we use that to + // detect outdated 18 alphas that don't yet include useSyncExternalStore. + // + // Longer term, we'll probably test this branch using an actual build + // of React 17. + jest.mock('react', () => { + const { + // eslint-disable-next-line no-unused-vars + startTransition: _, + // eslint-disable-next-line no-unused-vars + useSyncExternalStore: __, + ...otherExports + } = jest.requireActual('react'); + return otherExports; + }); + } + + React = require('react'); + ReactDOM = require('react-dom'); + ReactDOMClient = require('react-dom/client'); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + + const internalAct = require('internal-test-utils').act; + + // The internal act implementation doesn't batch updates by default, since + // it's mostly used to test concurrent mode. But since these tests run + // in both concurrent and legacy mode, I'm adding batching here. + act = cb => internalAct(() => ReactDOM.unstable_batchedUpdates(cb)); + + if (gate(flags => flags.source)) { + // The `shim/with-selector` module composes the main + // `use-sync-external-store` entrypoint. In the compiled artifacts, this + // is resolved to the `shim` implementation by our build config, but when + // running the tests against the source files, we need to tell Jest how to + // resolve it. Because this is a source module, this mock has no affect on + // the build tests. + jest.mock('use-sync-external-store/src/useSyncExternalStore', () => + jest.requireActual('use-sync-external-store/shim'), + ); + } + useSyncExternalStoreWithSelector = + require('use-sync-external-store/shim/with-selector').useSyncExternalStoreWithSelector; + }); + + function createRoot(container) { + // This wrapper function exists so we can test both legacy roots and + // concurrent roots. + if (gate(flags => !flags.enableUseSyncExternalStoreShim)) { + // The native implementation only exists in 18+, so we test using + // concurrent mode. + return ReactDOMClient.createRoot(container); + } else { + // For legacy mode, use ReactDOM.createRoot instead of ReactDOM.render + const root = ReactDOMClient.createRoot(container); + return { + render(children) { + root.render(children); + }, + }; + } + } + + function createExternalStore(initialState) { + const listeners = new Set(); + let currentState = initialState; + return { + set(text) { + currentState = text; + ReactDOM.unstable_batchedUpdates(() => { + listeners.forEach(listener => listener()); + }); + }, + subscribe(listener) { + listeners.add(listener); + return () => listeners.delete(listener); + }, + getState() { + return currentState; + }, + getSubscriberCount() { + return listeners.size; + }, + }; + } + + test('should call selector on change accessible segment', async () => { + const store = createExternalStore({a: '1', b: '2'}); + + const selectorFn = jest.fn(); + const selector = state => { + selectorFn(); + return state.a; + }; + + function App() { + const data = useSyncExternalStoreWithSelector( + store.subscribe, + store.getState, + null, + selector, + ); + return <>{data}; + } + + const container = document.createElement('div'); + const root = createRoot(container); + await act(() => { + root.render(); + }); + + expect(selectorFn).toHaveBeenCalledTimes(1); + + await expect(async () => { + await act(() => { + store.set({a: '2', b: '2'}); + }); + }).toWarnDev( + ReactFeatureFlags.enableUseRefAccessWarning + ? ['Warning: App: Unsafe read of a mutable value during render.'] + : [], + ); + + expect(selectorFn).toHaveBeenCalledTimes(2); + }); + + test('should not call selector if nothing changed', async () => { + const store = createExternalStore({a: '1', b: '2'}); + + const selectorFn = jest.fn(); + const selector = state => { + selectorFn(); + return state.a; + }; + + function App() { + const data = useSyncExternalStoreWithSelector( + store.subscribe, + store.getState, + null, + selector, + ); + return <>{data}; + } + + const container = document.createElement('div'); + const root = createRoot(container); + await act(() => { + root.render(); + }); + + expect(selectorFn).toHaveBeenCalledTimes(1); + + await act(() => { + store.set({a: '1', b: '2'}); + }); + + expect(selectorFn).toHaveBeenCalledTimes(1); + }); + + test('should not call selector on change not accessible segment', async () => { + const store = createExternalStore({a: '1', b: '2'}); + + const selectorFn = jest.fn(); + const selector = state => { + selectorFn(); + return state.a; + }; + + function App() { + const data = useSyncExternalStoreWithSelector( + store.subscribe, + store.getState, + null, + selector, + ); + return <>{data}; + } + + const container = document.createElement('div'); + const root = createRoot(container); + await act(() => { + root.render(); + }); + + expect(selectorFn).toHaveBeenCalledTimes(1); + + await act(() => { + store.set({a: '1', b: '3'}); + }); + + expect(selectorFn).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js b/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js index a7be5c0fe8fda..3e03e95fabbee 100644 --- a/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js +++ b/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js @@ -17,7 +17,7 @@ const {useRef, useEffect, useMemo, useDebugValue} = React; // Same as useSyncExternalStore, but supports selector and isEqual arguments. export function useSyncExternalStoreWithSelector( - subscribe: (() => void) => () => void, + subscribe: (onStoreChange: () => void) => () => void, getSnapshot: () => Snapshot, getServerSnapshot: void | null | (() => Snapshot), selector: (snapshot: Snapshot) => Selection, @@ -54,12 +54,44 @@ export function useSyncExternalStoreWithSelector( let hasMemo = false; let memoizedSnapshot; let memoizedSelection: Selection; + let lastUsedProps: string[] = []; + let hasAccessed = false; + const accessedProps: string[] = []; + const memoizedSelector = (nextSnapshot: Snapshot) => { + const getProxy = (): Snapshot => { + if ( + !(typeof nextSnapshot === 'object') || + typeof Proxy === 'undefined' + ) { + return nextSnapshot; + } + + const handler = { + get: (target: Snapshot, prop: string, receiver: any) => { + const propertyName = prop.toString(); + + if (accessedProps.indexOf(propertyName) === -1) { + accessedProps.push(propertyName); + } + + const value = Reflect.get(target, prop, receiver); + + return value; + }, + }; + + return (new Proxy(nextSnapshot, handler): any); + }; + if (!hasMemo) { // The first time the hook is called, there is no memoized result. hasMemo = true; memoizedSnapshot = nextSnapshot; - const nextSelection = selector(nextSnapshot); + const nextSelection = selector(getProxy()); + lastUsedProps = accessedProps; + hasAccessed = true; + if (isEqual !== undefined) { // Even if the selector has changed, the currently rendered selection // may be equal to the new selection. We should attempt to reuse the @@ -77,8 +109,37 @@ export function useSyncExternalStoreWithSelector( } // We may be able to reuse the previous invocation's result. - const prevSnapshot: Snapshot = (memoizedSnapshot: any); - const prevSelection: Selection = (memoizedSelection: any); + const prevSnapshot = memoizedSnapshot; + const prevSelection = memoizedSelection; + + const getChangedSegments = (): string[] | void => { + if ( + prevSnapshot === undefined || + !hasAccessed || + lastUsedProps.length === 0 + ) { + return undefined; + } + + const result: string[] = []; + + if ( + nextSnapshot !== null && + typeof nextSnapshot === 'object' && + prevSnapshot !== null && + typeof prevSnapshot === 'object' + ) { + for (let i = 0; i < lastUsedProps.length; i++) { + const segmentName = lastUsedProps[i]; + + if (nextSnapshot[segmentName] !== prevSnapshot[segmentName]) { + result.push(segmentName); + } + } + } + + return result; + }; if (is(prevSnapshot, nextSnapshot)) { // The snapshot is the same as last time. Reuse the previous selection. @@ -86,22 +147,27 @@ export function useSyncExternalStoreWithSelector( } // The snapshot has changed, so we need to compute a new selection. - const nextSelection = selector(nextSnapshot); - - // If a custom isEqual function is provided, use that to check if the data - // has changed. If it hasn't, return the previous selection. That signals - // to React that the selections are conceptually equal, and we can bail - // out of rendering. - if (isEqual !== undefined && isEqual(prevSelection, nextSelection)) { - // The snapshot still has changed, so make sure to update to not keep - // old references alive + + const changedSegments = getChangedSegments(); + if (changedSegments === undefined || changedSegments.length > 0) { + const nextSelection = selector(getProxy()); + lastUsedProps = accessedProps; + hasAccessed = true; + + // If a custom isEqual function is provided, use that to check if the data + // has changed. If it hasn't, return the previous selection. That signals + // to React that the selections are conceptually equal, and we can bail + // out of rendering. + if (isEqual !== undefined && isEqual(prevSelection, nextSelection)) { + return prevSelection; + } + memoizedSnapshot = nextSnapshot; + memoizedSelection = nextSelection; + return nextSelection; + } else { return prevSelection; } - - memoizedSnapshot = nextSnapshot; - memoizedSelection = nextSelection; - return nextSelection; }; // Assigning this to a constant so that Flow knows it can't change. const maybeGetServerSnapshot = From aeab8bcb15bcb830fcfbb12bc071dd2e71c35d3c Mon Sep 17 00:00:00 2001 From: costrings-frontend-dev Date: Fri, 10 Jan 2025 22:25:16 +0530 Subject: [PATCH 2/6] removed eslint directives --- .../src/__tests__/useSyncExternalStoreWithSelector-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js index 65fc8c4c959b4..529d117c3e61e 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js @@ -29,9 +29,7 @@ describe('useSyncExternalStoreWithSelector', () => { // of React 17. jest.mock('react', () => { const { - // eslint-disable-next-line no-unused-vars startTransition: _, - // eslint-disable-next-line no-unused-vars useSyncExternalStore: __, ...otherExports } = jest.requireActual('react'); From dd01f2d2a36939f114fe39bb3c8c205ee521d864 Mon Sep 17 00:00:00 2001 From: costrings-frontend-dev Date: Fri, 10 Jan 2025 23:03:11 +0530 Subject: [PATCH 3/6] remove unnecessary async call --- .../useSyncExternalStoreWithSelector-test.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js index 529d117c3e61e..b5ac949a5a0a1 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js @@ -15,6 +15,7 @@ let ReactDOM; let ReactDOMClient; let ReactFeatureFlags; let act; +let assertLog; describe('useSyncExternalStoreWithSelector', () => { beforeEach(() => { @@ -29,7 +30,9 @@ describe('useSyncExternalStoreWithSelector', () => { // of React 17. jest.mock('react', () => { const { + // eslint-disable-next-line no-unused-vars startTransition: _, + // eslint-disable-next-line no-unused-vars useSyncExternalStore: __, ...otherExports } = jest.requireActual('react'); @@ -43,6 +46,7 @@ describe('useSyncExternalStoreWithSelector', () => { ReactFeatureFlags = require('shared/ReactFeatureFlags'); const internalAct = require('internal-test-utils').act; + assertLog = internalAct.assertLog; // The internal act implementation doesn't batch updates by default, since // it's mostly used to test concurrent mode. But since these tests run @@ -132,15 +136,9 @@ describe('useSyncExternalStoreWithSelector', () => { expect(selectorFn).toHaveBeenCalledTimes(1); - await expect(async () => { - await act(() => { - store.set({a: '2', b: '2'}); - }); - }).toWarnDev( - ReactFeatureFlags.enableUseRefAccessWarning - ? ['Warning: App: Unsafe read of a mutable value during render.'] - : [], - ); + await act(() => { + store.set({a: '2', b: '2'}); + }); expect(selectorFn).toHaveBeenCalledTimes(2); }); From c4c9e8759a1f89d8cbe98275d9fe413823d4e6cb Mon Sep 17 00:00:00 2001 From: costrings-frontend-dev Date: Fri, 10 Jan 2025 23:06:45 +0530 Subject: [PATCH 4/6] removed unused variables --- .../src/__tests__/useSyncExternalStoreWithSelector-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js index b5ac949a5a0a1..1024fa4d0f637 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js @@ -43,10 +43,8 @@ describe('useSyncExternalStoreWithSelector', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); - ReactFeatureFlags = require('shared/ReactFeatureFlags'); const internalAct = require('internal-test-utils').act; - assertLog = internalAct.assertLog; // The internal act implementation doesn't batch updates by default, since // it's mostly used to test concurrent mode. But since these tests run From dbc1822a475d5b4ffe9dbff94a89d9967bec39c8 Mon Sep 17 00:00:00 2001 From: costrings-frontend-dev Date: Fri, 10 Jan 2025 23:09:19 +0530 Subject: [PATCH 5/6] removed declare variables --- .../src/__tests__/useSyncExternalStoreWithSelector-test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js index 1024fa4d0f637..e808d1a10b4aa 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js @@ -13,9 +13,7 @@ let useSyncExternalStoreWithSelector; let React; let ReactDOM; let ReactDOMClient; -let ReactFeatureFlags; let act; -let assertLog; describe('useSyncExternalStoreWithSelector', () => { beforeEach(() => { @@ -43,7 +41,6 @@ describe('useSyncExternalStoreWithSelector', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); - const internalAct = require('internal-test-utils').act; // The internal act implementation doesn't batch updates by default, since From 85ef2ab76a478a66a7976f86fbf6855272c72298 Mon Sep 17 00:00:00 2001 From: costrings-frontend-dev Date: Fri, 10 Jan 2025 23:12:06 +0530 Subject: [PATCH 6/6] revert directives --- .../src/__tests__/useSyncExternalStoreWithSelector-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js index e808d1a10b4aa..c17e6e39ce124 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreWithSelector-test.js @@ -28,9 +28,7 @@ describe('useSyncExternalStoreWithSelector', () => { // of React 17. jest.mock('react', () => { const { - // eslint-disable-next-line no-unused-vars startTransition: _, - // eslint-disable-next-line no-unused-vars useSyncExternalStore: __, ...otherExports } = jest.requireActual('react');