From ab84793cbc6ec2db5cce83e268d8e4adbe56c131 Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Wed, 3 Apr 2024 12:26:44 -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 | 1 - .../shared/forks/ReactFeatureFlags.www.js | 1 - .../useSyncExternalStoreNative-test.js | 1 - .../useSyncExternalStoreShared-test.js | 52 ++------ 13 files changed, 13 insertions(+), 263 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index b548948344d14..0d86c2e1c5750 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, @@ -2330,90 +2329,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 a55e864806457..c527d069dcaa5 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -195,8 +195,6 @@ export const enableRenderableContext = true; // 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 2dfb0f2bd2a0d..50c65b0038e16 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js @@ -25,6 +25,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 62b554010caa2..004c76d747569 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -27,7 +27,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 f89969af59b9c..79f77bdc052ca 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -91,7 +91,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 f93977ed1925e..cef2b788b73ce 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -48,8 +48,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 2b85343516ab7..d12550dabbc1d 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -43,7 +43,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 9a757423fe7d7..3bfc66a0069ba 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -50,8 +50,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 96921a847d93c..b7e953047e77d 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -14,7 +14,6 @@ // with the __VARIANT__ set to `true`, and once set to `false`. export const disableIEWorkarounds = __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 39fcd219eae38..3057542f39740 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -18,7 +18,6 @@ export const { disableIEWorkarounds, enableTrustedTypesIntegration, enableDebugTracing, - enableUseRefAccessWarning, enableLazyContextPropagation, enableUnifiedSyncLane, enableRetryLaneExpiration, 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');