Skip to content

Commit

Permalink
Cleanup enableUseRefAccessWarning flag
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@kassens
kassens committed Apr 3, 2024
1 parent 7a2609e commit ab84793
Show file tree
Hide file tree
Showing 13 changed files with 13 additions and 263 deletions.
86 changes: 3 additions & 83 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import {
enableDebugTracing,
enableSchedulingProfiler,
enableCache,
enableUseRefAccessWarning,
enableLazyContextPropagation,
enableTransitionTracing,
enableUseMemoCacheHook,
Expand Down Expand Up @@ -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<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} {
Expand Down
125 changes: 0 additions & 125 deletions packages/react-reconciler/src/__tests__/useRef-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 0 additions & 2 deletions packages/shared/ReactFeatureFlags.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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__;
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export const {
enableInfiniteRenderLoopDetection,
enableRenderableContext,
enableUnifiedSyncLane,
enableUseRefAccessWarning,
passChildrenWhenCloningPersistedNodes,
useModernStrictMode,
} = dynamicFlags;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-oss.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 0 additions & 2 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www-dynamic.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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__;
Expand Down
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ export const {
disableIEWorkarounds,
enableTrustedTypesIntegration,
enableDebugTracing,
enableUseRefAccessWarning,
enableLazyContextPropagation,
enableUnifiedSyncLane,
enableRetryLaneExpiration,
Expand Down
1 change: 0 additions & 1 deletion packages/use-sync-external-store/src/__tests__/useSyncExternalStoreNative-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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});

Expand Down
Loading

0 comments on commit ab84793

Please sign in to comment.