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.

Author: kassens

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<T>(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<T>(initialValue: T): {current: T} {

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(<Example />);
-      });
-    });
-
     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(<Example />);
-      });
-
-      // Should not warn again on update.
-      shouldExpectWarning = false;
-      await act(() => {
-        ReactNoop.render(<Example />);
-      });
-    });
-
-    // @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(<Example />);
-      });
-    });
-
-    // @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(<Example />);
-      });
-    });
-
     it('should not warn about reads or writes within effect', async () => {
       function Example() {
         const ref = useRef(123);

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;
 

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__;

packages/shared/forks/ReactFeatureFlags.native-fb.js

@@ -28,7 +28,6 @@ export const {
   enableInfiniteRenderLoopDetection,
   enableRenderableContext,
   enableUnifiedSyncLane,
-  enableUseRefAccessWarning,
   passChildrenWhenCloningPersistedNodes,
   useModernStrictMode,
 } = dynamicFlags;

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;

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;

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;
 

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;

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__;

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);

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});