From ceeb1b4d26d0c58ea455bcc976811e5391d359ae Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Mon, 1 Apr 2024 13:23:31 -0400 Subject: [PATCH] Cleanup enableUseRefAccessWarning flag I don't think this flag has a path forward in the current implementation. The detection by stack trace is too brittle to detect the lazy initialization pattern reliably (see e.g. some internal tests that expect the warning because they use lazy intialization, but a slightly different pattern then the expected pattern. I think a new version of this could be to fully ban ref access during render with an alternative API for the exceptional cases that today require ref access during render. --- .../react-reconciler/src/ReactFiberHooks.js | 86 +----------- .../src/__tests__/useRef-test.internal.js | 125 ------------------ packages/shared/ReactFeatureFlags.js | 2 - .../ReactFeatureFlags.native-fb-dynamic.js | 1 - .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 2 - ...actFeatureFlags.test-renderer.native-fb.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 2 - .../forks/ReactFeatureFlags.www-dynamic.js | 2 - .../shared/forks/ReactFeatureFlags.www.js | 4 +- .../useSyncExternalStoreNative-test.js | 1 - .../useSyncExternalStoreShared-test.js | 52 ++------ 13 files changed, 15 insertions(+), 265 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 127656768cf30..6af6817a38f70 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -33,7 +33,6 @@ import { enableDebugTracing, enableSchedulingProfiler, enableCache, - enableUseRefAccessWarning, enableLazyContextPropagation, enableTransitionTracing, enableUseMemoCacheHook, @@ -2329,90 +2328,11 @@ function createEffectInstance(): EffectInstance { return {destroy: undefined}; } -let stackContainsErrorMessage: boolean | null = null; - -function getCallerStackFrame(): string { - // eslint-disable-next-line react-internal/prod-error-codes - const stackFrames = new Error('Error message').stack.split('\n'); - - // Some browsers (e.g. Chrome) include the error message in the stack - // but others (e.g. Firefox) do not. - if (stackContainsErrorMessage === null) { - stackContainsErrorMessage = stackFrames[0].includes('Error message'); - } - - return stackContainsErrorMessage - ? stackFrames.slice(3, 4).join('\n') - : stackFrames.slice(2, 3).join('\n'); -} - function mountRef(initialValue: T): {current: T} { const hook = mountWorkInProgressHook(); - if (enableUseRefAccessWarning) { - if (__DEV__) { - // Support lazy initialization pattern shown in docs. - // We need to store the caller stack frame so that we don't warn on subsequent renders. - let hasBeenInitialized = initialValue != null; - let lazyInitGetterStack = null; - let didCheckForLazyInit = false; - - // Only warn once per component+hook. - let didWarnAboutRead = false; - let didWarnAboutWrite = false; - - let current = initialValue; - const ref = { - get current() { - if (!hasBeenInitialized) { - didCheckForLazyInit = true; - lazyInitGetterStack = getCallerStackFrame(); - } else if (currentlyRenderingFiber !== null && !didWarnAboutRead) { - if ( - lazyInitGetterStack === null || - lazyInitGetterStack !== getCallerStackFrame() - ) { - didWarnAboutRead = true; - console.warn( - '%s: Unsafe read of a mutable value during render.\n\n' + - 'Reading from a ref during render is only safe if:\n' + - '1. The ref value has not been updated, or\n' + - '2. The ref holds a lazily-initialized value that is only set once.\n', - getComponentNameFromFiber(currentlyRenderingFiber) || 'Unknown', - ); - } - } - return current; - }, - set current(value: any) { - if (currentlyRenderingFiber !== null && !didWarnAboutWrite) { - if (hasBeenInitialized || !didCheckForLazyInit) { - didWarnAboutWrite = true; - console.warn( - '%s: Unsafe write of a mutable value during render.\n\n' + - 'Writing to a ref during render is only safe if the ref holds ' + - 'a lazily-initialized value that is only set once.\n', - getComponentNameFromFiber(currentlyRenderingFiber) || 'Unknown', - ); - } - } - - hasBeenInitialized = true; - current = value; - }, - }; - Object.seal(ref); - hook.memoizedState = ref; - return ref; - } else { - const ref = {current: initialValue}; - hook.memoizedState = ref; - return ref; - } - } else { - const ref = {current: initialValue}; - hook.memoizedState = ref; - return ref; - } + const ref = {current: initialValue}; + hook.memoizedState = ref; + return ref; } function updateRef(initialValue: T): {current: T} { diff --git a/packages/react-reconciler/src/__tests__/useRef-test.internal.js b/packages/react-reconciler/src/__tests__/useRef-test.internal.js index 9c918b961ad31..eef278f4d7789 100644 --- a/packages/react-reconciler/src/__tests__/useRef-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useRef-test.internal.js @@ -160,24 +160,6 @@ describe('useRef', () => { }); }); - // @gate enableUseRefAccessWarning - it('should warn about reads during render', async () => { - function Example() { - const ref = useRef(123); - let value; - expect(() => { - value = ref.current; - }).toWarnDev([ - 'Example: Unsafe read of a mutable value during render.', - ]); - return value; - } - - await act(() => { - ReactNoop.render(); - }); - }); - it('should not warn about lazy init during render', async () => { function Example() { const ref1 = useRef(null); @@ -221,113 +203,6 @@ describe('useRef', () => { }); }); - // @gate enableUseRefAccessWarning - it('should warn about unconditional lazy init during render', async () => { - function Example() { - const ref1 = useRef(null); - const ref2 = useRef(undefined); - - if (shouldExpectWarning) { - expect(() => { - ref1.current = 123; - }).toWarnDev([ - 'Example: Unsafe write of a mutable value during render', - ]); - expect(() => { - ref2.current = 123; - }).toWarnDev([ - 'Example: Unsafe write of a mutable value during render', - ]); - } else { - ref1.current = 123; - ref1.current = 123; - } - - // But only warn once - ref1.current = 345; - ref1.current = 345; - - return null; - } - - let shouldExpectWarning = true; - await act(() => { - ReactNoop.render(); - }); - - // Should not warn again on update. - shouldExpectWarning = false; - await act(() => { - ReactNoop.render(); - }); - }); - - // @gate enableUseRefAccessWarning - it('should warn about reads to ref after lazy init pattern', async () => { - function Example() { - const ref1 = useRef(null); - const ref2 = useRef(undefined); - - // Read 1: safe because lazy init: - if (ref1.current === null) { - ref1.current = 123; - } - if (ref2.current === undefined) { - ref2.current = 123; - } - - let value; - expect(() => { - value = ref1.current; - }).toWarnDev(['Example: Unsafe read of a mutable value during render']); - expect(() => { - value = ref2.current; - }).toWarnDev(['Example: Unsafe read of a mutable value during render']); - - // But it should only warn once. - value = ref1.current; - value = ref2.current; - - return value; - } - - await act(() => { - ReactNoop.render(); - }); - }); - - // @gate enableUseRefAccessWarning - it('should warn about writes to ref after lazy init pattern', async () => { - function Example() { - const ref1 = useRef(null); - const ref2 = useRef(undefined); - // Read: safe because lazy init: - if (ref1.current === null) { - ref1.current = 123; - } - if (ref2.current === undefined) { - ref2.current = 123; - } - - expect(() => { - ref1.current = 456; - }).toWarnDev([ - 'Example: Unsafe write of a mutable value during render', - ]); - expect(() => { - ref2.current = 456; - }).toWarnDev([ - 'Example: Unsafe write of a mutable value during render', - ]); - - return null; - } - - await act(() => { - ReactNoop.render(); - }); - }); - it('should not warn about reads or writes within effect', async () => { function Example() { const ref = useRef(123); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 4d4fc361f4879..6406a2a9872c3 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -193,8 +193,6 @@ export const enableRenderableContext = __NEXT_MAJOR__; // when we plan to enable them. // ----------------------------------------------------------------------------- -export const enableUseRefAccessWarning = false; - // Enables time slicing for updates that aren't wrapped in startTransition. export const forceConcurrentByDefaultForTesting = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js index be82df80b8217..d4eca009ca6bb 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js @@ -26,6 +26,5 @@ export const enableDeferRootSchedulingToMicrotask = __VARIANT__; export const enableInfiniteRenderLoopDetection = __VARIANT__; export const enableRenderableContext = __VARIANT__; export const enableUnifiedSyncLane = __VARIANT__; -export const enableUseRefAccessWarning = __VARIANT__; export const passChildrenWhenCloningPersistedNodes = __VARIANT__; export const useModernStrictMode = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 13e8d00c5b3b3..4d702b097f5b9 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -28,7 +28,6 @@ export const { enableInfiniteRenderLoopDetection, enableRenderableContext, enableUnifiedSyncLane, - enableUseRefAccessWarning, passChildrenWhenCloningPersistedNodes, useModernStrictMode, } = dynamicFlags; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 904f165c214cb..8ed26e459d057 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -93,7 +93,6 @@ export const enableRetryLaneExpiration = false; export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const enableUseRefAccessWarning = false; export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = false; export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index df84dd4d4cc60..7a75dcae2cda5 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -49,8 +49,6 @@ export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const enableUseRefAccessWarning = false; - export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = false; export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index 879806d751575..a1417aa3d45c4 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -44,7 +44,6 @@ export const enableCPUSuspense = true; export const enableUseMemoCacheHook = true; export const enableUseEffectEventHook = false; export const favorSafetyOverHydrationPerf = true; -export const enableUseRefAccessWarning = false; export const enableInfiniteRenderLoopDetection = false; export const enableRenderableContext = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 137852fad7c5c..69dc091d67d09 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -51,8 +51,6 @@ export const retryLaneExpirationMs = 5000; export const syncLaneExpirationMs = 250; export const transitionLaneExpirationMs = 5000; -export const enableUseRefAccessWarning = false; - export const disableSchedulerTimeoutInWorkLoop = false; export const enableLazyContextPropagation = false; export const enableLegacyHidden = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 95b9927092fc6..5956ee6dedf16 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -16,8 +16,6 @@ export const disableInputAttributeSyncing = __VARIANT__; export const disableIEWorkarounds = __VARIANT__; export const enableBigIntSupport = __VARIANT__; -export const enableLegacyFBSupport = __VARIANT__; -export const enableUseRefAccessWarning = __VARIANT__; export const disableSchedulerTimeoutInWorkLoop = __VARIANT__; export const enableLazyContextPropagation = __VARIANT__; export const forceConcurrentByDefaultForTesting = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 79ff994a0ad2d..73cdf70efd5ad 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -19,9 +19,7 @@ export const { disableIEWorkarounds, enableBigIntSupport, enableTrustedTypesIntegration, - enableLegacyFBSupport, enableDebugTracing, - enableUseRefAccessWarning, enableLazyContextPropagation, enableUnifiedSyncLane, enableRetryLaneExpiration, @@ -118,5 +116,7 @@ export const disableLegacyMode = false; export const disableDOMTestUtils = false; +export const enableLegacyFBSupport = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreNative-test.js b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreNative-test.js index fef2d1e76f581..24d9e96d29d5c 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreNative-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreNative-test.js @@ -124,7 +124,6 @@ describe('useSyncExternalStore (userspace shim, server rendering)', () => { expect(root).toMatchRenderedOutput('client'); }); - // @gate !(enableUseRefAccessWarning && __DEV__) test('Using isEqual to bailout', async () => { const store = createExternalStore({a: 0, b: 0}); 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 07058d4107158..b8f66d15c8907 100644 --- a/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js +++ b/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js @@ -14,7 +14,6 @@ let useSyncExternalStoreWithSelector; let React; let ReactDOM; let ReactDOMClient; -let ReactFeatureFlags; let Scheduler; let act; let useState; @@ -54,7 +53,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMClient = require('react-dom/client'); - ReactFeatureFlags = require('shared/ReactFeatureFlags'); Scheduler = require('scheduler'); useState = React.useState; useEffect = React.useEffect; @@ -673,8 +671,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { }); describe('extra features implemented in user-space', () => { - // The selector implementation uses the lazy ref initialization pattern - // @gate !(enableUseRefAccessWarning && __DEV__) it('memoized selectors are only called once per update', async () => { const store = createExternalStore({a: 0, b: 0}); @@ -716,8 +712,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(container.textContent).toEqual('A1'); }); - // The selector implementation uses the lazy ref initialization pattern - // @gate !(enableUseRefAccessWarning && __DEV__) it('Using isEqual to bailout', async () => { const store = createExternalStore({a: 0, b: 0}); @@ -857,8 +851,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { expect(container.textContent).toEqual('UPDATED'); }); - // The selector implementation uses the lazy ref initialization pattern - // @gate !(enableUseRefAccessWarning && __DEV__) it('compares selection to rendered selection even if selector changes', async () => { const store = createExternalStore({items: ['A', 'B']}); @@ -980,27 +972,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { if (__DEV__ && gate(flags => flags.enableUseSyncExternalStoreShim)) { // In 17, the error is re-thrown in DEV. - await expect(async () => { - await expect(async () => { - await act(() => { - store.set({}); - }); - }).rejects.toThrow('Malformed state'); - }).toWarnDev( - ReactFeatureFlags.enableUseRefAccessWarning - ? ['Warning: App: Unsafe read of a mutable value during render.'] - : [], - ); - } else { await expect(async () => { await act(() => { store.set({}); }); - }).toWarnDev( - ReactFeatureFlags.enableUseRefAccessWarning - ? ['Warning: App: Unsafe read of a mutable value during render.'] - : [], - ); + }).rejects.toThrow('Malformed state'); + } else { + await act(() => { + store.set({}); + }); } expect(container.textContent).toEqual('Malformed state'); @@ -1041,27 +1021,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => { if (__DEV__ && gate(flags => flags.enableUseSyncExternalStoreShim)) { // In 17, the error is re-thrown in DEV. - await expect(async () => { - await expect(async () => { - await act(() => { - store.set({}); - }); - }).rejects.toThrow('Malformed state'); - }).toWarnDev( - ReactFeatureFlags.enableUseRefAccessWarning - ? ['Warning: App: Unsafe read of a mutable value during render.'] - : [], - ); - } else { await expect(async () => { await act(() => { store.set({}); }); - }).toWarnDev( - ReactFeatureFlags.enableUseRefAccessWarning - ? ['Warning: App: Unsafe read of a mutable value during render.'] - : [], - ); + }).rejects.toThrow('Malformed state'); + } else { + await act(() => { + store.set({}); + }); } expect(container.textContent).toEqual('Malformed state');