Cleanup enableUseRefAccessWarning flag (#28699)
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.
Jan Kassens committed
Apr 3, 2024 at 13:35 UTC
20e710aeab3e03809c82d134171986ea270026a0
13 files changed
+13
-263
packages/react-reconciler/src/ReactFiberHooks.js
+3
-83
@@ -33,7 +33,6 @@ import {
33
enableDebugTracing,
34
enableSchedulingProfiler,
35
enableCache,
36
- enableUseRefAccessWarning,
36
enableLazyContextPropagation,
37
enableTransitionTracing,
38
enableUseMemoCacheHook,
@@ -2330,90 +2329,11 @@ function createEffectInstance(): EffectInstance {
2329
return {destroy: undefined};
2330
}
2331
2333
-let stackContainsErrorMessage: boolean | null = null;
2334
-
2335
-function getCallerStackFrame(): string {
2336
- // eslint-disable-next-line react-internal/prod-error-codes
2337
- const stackFrames = new Error('Error message').stack.split('\n');
2338
-
2339
- // Some browsers (e.g. Chrome) include the error message in the stack
2340
- // but others (e.g. Firefox) do not.
2341
- if (stackContainsErrorMessage === null) {
2342
- stackContainsErrorMessage = stackFrames[0].includes('Error message');
2343
- }
2344
-
2345
- return stackContainsErrorMessage
2346
- ? stackFrames.slice(3, 4).join('\n')
2347
- : stackFrames.slice(2, 3).join('\n');
2348
-}
2349
-
2332
function mountRef<T>(initialValue: T): {current: T} {
2333
const hook = mountWorkInProgressHook();
2352
- if (enableUseRefAccessWarning) {
2353
- if (__DEV__) {
2354
- // Support lazy initialization pattern shown in docs.
2355
- // We need to store the caller stack frame so that we don't warn on subsequent renders.
2356
- let hasBeenInitialized = initialValue != null;
2357
- let lazyInitGetterStack = null;
2358
- let didCheckForLazyInit = false;
2359
-
2360
- // Only warn once per component+hook.
2361
- let didWarnAboutRead = false;
2362
- let didWarnAboutWrite = false;
2363
-
2364
- let current = initialValue;
2365
- const ref = {
2366
- get current() {
2367
- if (!hasBeenInitialized) {
2368
- didCheckForLazyInit = true;
2369
- lazyInitGetterStack = getCallerStackFrame();
2370
- } else if (currentlyRenderingFiber !== null && !didWarnAboutRead) {
2371
- if (
2372
- lazyInitGetterStack === null ||
2373
- lazyInitGetterStack !== getCallerStackFrame()
2374
- ) {
2375
- didWarnAboutRead = true;
2376
- console.warn(
2377
- '%s: Unsafe read of a mutable value during render.\n\n' +
2378
- 'Reading from a ref during render is only safe if:\n' +
2379
- '1. The ref value has not been updated, or\n' +
2380
- '2. The ref holds a lazily-initialized value that is only set once.\n',
2381
- getComponentNameFromFiber(currentlyRenderingFiber) || 'Unknown',
2382
- );
2383
- }
2384
- }
2385
- return current;
2386
- },
2387
- set current(value: any) {
2388
- if (currentlyRenderingFiber !== null && !didWarnAboutWrite) {
2389
- if (hasBeenInitialized || !didCheckForLazyInit) {
2390
- didWarnAboutWrite = true;
2391
- console.warn(
2392
- '%s: Unsafe write of a mutable value during render.\n\n' +
2393
- 'Writing to a ref during render is only safe if the ref holds ' +
2394
- 'a lazily-initialized value that is only set once.\n',
2395
- getComponentNameFromFiber(currentlyRenderingFiber) || 'Unknown',
2396
- );
2397
- }
2398
- }
2399
-
2400
- hasBeenInitialized = true;
2401
- current = value;
2402
- },
2403
- };
2404
- Object.seal(ref);
2405
- hook.memoizedState = ref;
2406
- return ref;
2407
- } else {
2408
- const ref = {current: initialValue};
2409
- hook.memoizedState = ref;
2410
- return ref;
2411
- }
2412
- } else {
2413
- const ref = {current: initialValue};
2414
- hook.memoizedState = ref;
2415
- return ref;
2416
- }
2334
+ const ref = {current: initialValue};
2335
+ hook.memoizedState = ref;
2336
+ return ref;
2337
}
2338
2339
function updateRef<T>(initialValue: T): {current: T} {
packages/react-reconciler/src/__tests__/useRef-test.internal.js
-125
@@ -160,24 +160,6 @@ describe('useRef', () => {
160
});
161
});
162
163
- // @gate enableUseRefAccessWarning
164
- it('should warn about reads during render', async () => {
165
- function Example() {
166
- const ref = useRef(123);
167
- let value;
168
- expect(() => {
169
- value = ref.current;
170
- }).toWarnDev([
171
- 'Example: Unsafe read of a mutable value during render.',
172
- ]);
173
- return value;
174
- }
175
-
176
- await act(() => {
177
- ReactNoop.render(<Example />);
178
- });
179
- });
180
-
163
it('should not warn about lazy init during render', async () => {
164
function Example() {
165
const ref1 = useRef(null);
@@ -221,113 +203,6 @@ describe('useRef', () => {
203
});
204
});
205
224
- // @gate enableUseRefAccessWarning
225
- it('should warn about unconditional lazy init during render', async () => {
226
- function Example() {
227
- const ref1 = useRef(null);
228
- const ref2 = useRef(undefined);
229
-
230
- if (shouldExpectWarning) {
231
- expect(() => {
232
- ref1.current = 123;
233
- }).toWarnDev([
234
- 'Example: Unsafe write of a mutable value during render',
235
- ]);
236
- expect(() => {
237
- ref2.current = 123;
238
- }).toWarnDev([
239
- 'Example: Unsafe write of a mutable value during render',
240
- ]);
241
- } else {
242
- ref1.current = 123;
243
- ref1.current = 123;
244
- }
245
-
246
- // But only warn once
247
- ref1.current = 345;
248
- ref1.current = 345;
249
-
250
- return null;
251
- }
252
-
253
- let shouldExpectWarning = true;
254
- await act(() => {
255
- ReactNoop.render(<Example />);
256
- });
257
-
258
- // Should not warn again on update.
259
- shouldExpectWarning = false;
260
- await act(() => {
261
- ReactNoop.render(<Example />);
262
- });
263
- });
264
-
265
- // @gate enableUseRefAccessWarning
266
- it('should warn about reads to ref after lazy init pattern', async () => {
267
- function Example() {
268
- const ref1 = useRef(null);
269
- const ref2 = useRef(undefined);
270
-
271
- // Read 1: safe because lazy init:
272
- if (ref1.current === null) {
273
- ref1.current = 123;
274
- }
275
- if (ref2.current === undefined) {
276
- ref2.current = 123;
277
- }
278
-
279
- let value;
280
- expect(() => {
281
- value = ref1.current;
282
- }).toWarnDev(['Example: Unsafe read of a mutable value during render']);
283
- expect(() => {
284
- value = ref2.current;
285
- }).toWarnDev(['Example: Unsafe read of a mutable value during render']);
286
-
287
- // But it should only warn once.
288
- value = ref1.current;
289
- value = ref2.current;
290
-
291
- return value;
292
- }
293
-
294
- await act(() => {
295
- ReactNoop.render(<Example />);
296
- });
297
- });
298
-
299
- // @gate enableUseRefAccessWarning
300
- it('should warn about writes to ref after lazy init pattern', async () => {
301
- function Example() {
302
- const ref1 = useRef(null);
303
- const ref2 = useRef(undefined);
304
- // Read: safe because lazy init:
305
- if (ref1.current === null) {
306
- ref1.current = 123;
307
- }
308
- if (ref2.current === undefined) {
309
- ref2.current = 123;
310
- }
311
-
312
- expect(() => {
313
- ref1.current = 456;
314
- }).toWarnDev([
315
- 'Example: Unsafe write of a mutable value during render',
316
- ]);
317
- expect(() => {
318
- ref2.current = 456;
319
- }).toWarnDev([
320
- 'Example: Unsafe write of a mutable value during render',
321
- ]);
322
-
323
- return null;
324
- }
325
-
326
- await act(() => {
327
- ReactNoop.render(<Example />);
328
- });
329
- });
330
-
206
it('should not warn about reads or writes within effect', async () => {
207
function Example() {
208
const ref = useRef(123);
packages/shared/ReactFeatureFlags.js
-2
@@ -195,8 +195,6 @@ export const enableRenderableContext = true;
195
// when we plan to enable them.
196
// -----------------------------------------------------------------------------
197
198
-export const enableUseRefAccessWarning = false;
199
-
198
// Enables time slicing for updates that aren't wrapped in startTransition.
199
export const forceConcurrentByDefaultForTesting = false;
200
packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js
-1
@@ -25,6 +25,5 @@ export const enableDeferRootSchedulingToMicrotask = __VARIANT__;
25
export const enableInfiniteRenderLoopDetection = __VARIANT__;
26
export const enableRenderableContext = __VARIANT__;
27
export const enableUnifiedSyncLane = __VARIANT__;
28
-export const enableUseRefAccessWarning = __VARIANT__;
28
export const passChildrenWhenCloningPersistedNodes = __VARIANT__;
29
export const useModernStrictMode = __VARIANT__;
packages/shared/forks/ReactFeatureFlags.native-fb.js
-1
@@ -27,7 +27,6 @@ export const {
27
enableInfiniteRenderLoopDetection,
28
enableRenderableContext,
29
enableUnifiedSyncLane,
30
- enableUseRefAccessWarning,
30
passChildrenWhenCloningPersistedNodes,
31
useModernStrictMode,
32
} = dynamicFlags;
packages/shared/forks/ReactFeatureFlags.native-oss.js
-1
@@ -91,7 +91,6 @@ export const enableRetryLaneExpiration = false;
91
export const retryLaneExpirationMs = 5000;
92
export const syncLaneExpirationMs = 250;
93
export const transitionLaneExpirationMs = 5000;
94
-export const enableUseRefAccessWarning = false;
94
export const disableSchedulerTimeoutInWorkLoop = false;
95
export const enableLazyContextPropagation = false;
96
export const enableLegacyHidden = false;
packages/shared/forks/ReactFeatureFlags.test-renderer.js
-2
@@ -48,8 +48,6 @@ export const retryLaneExpirationMs = 5000;
48
export const syncLaneExpirationMs = 250;
49
export const transitionLaneExpirationMs = 5000;
50
51
-export const enableUseRefAccessWarning = false;
52
-
51
export const disableSchedulerTimeoutInWorkLoop = false;
52
export const enableLazyContextPropagation = false;
53
export const enableLegacyHidden = false;
packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js
-1
@@ -43,7 +43,6 @@ export const enableCPUSuspense = true;
43
export const enableUseMemoCacheHook = true;
44
export const enableUseEffectEventHook = false;
45
export const favorSafetyOverHydrationPerf = true;
46
-export const enableUseRefAccessWarning = false;
46
export const enableInfiniteRenderLoopDetection = false;
47
export const enableRenderableContext = false;
48
packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
-2
@@ -50,8 +50,6 @@ export const retryLaneExpirationMs = 5000;
50
export const syncLaneExpirationMs = 250;
51
export const transitionLaneExpirationMs = 5000;
52
53
-export const enableUseRefAccessWarning = false;
54
-
53
export const disableSchedulerTimeoutInWorkLoop = false;
54
export const enableLazyContextPropagation = false;
55
export const enableLegacyHidden = false;
packages/shared/forks/ReactFeatureFlags.www-dynamic.js
-1
@@ -14,7 +14,6 @@
14
// with the __VARIANT__ set to `true`, and once set to `false`.
15
16
export const disableIEWorkarounds = __VARIANT__;
17
-export const enableUseRefAccessWarning = __VARIANT__;
17
export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
18
export const enableLazyContextPropagation = __VARIANT__;
19
export const forceConcurrentByDefaultForTesting = __VARIANT__;
packages/shared/forks/ReactFeatureFlags.www.js
-1
@@ -18,7 +18,6 @@ export const {
18
disableIEWorkarounds,
19
enableTrustedTypesIntegration,
20
enableDebugTracing,
21
- enableUseRefAccessWarning,
21
enableLazyContextPropagation,
22
enableUnifiedSyncLane,
23
enableRetryLaneExpiration,
packages/use-sync-external-store/src/__tests__/useSyncExternalStoreNative-test.js
-1
@@ -124,7 +124,6 @@ describe('useSyncExternalStore (userspace shim, server rendering)', () => {
124
expect(root).toMatchRenderedOutput('client');
125
});
126
127
- // @gate !(enableUseRefAccessWarning && __DEV__)
127
test('Using isEqual to bailout', async () => {
128
const store = createExternalStore({a: 0, b: 0});
129
packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
+10
-42
@@ -14,7 +14,6 @@ let useSyncExternalStoreWithSelector;
14
let React;
15
let ReactDOM;
16
let ReactDOMClient;
17
-let ReactFeatureFlags;
17
let Scheduler;
18
let act;
19
let useState;
@@ -54,7 +53,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
53
React = require('react');
54
ReactDOM = require('react-dom');
55
ReactDOMClient = require('react-dom/client');
57
- ReactFeatureFlags = require('shared/ReactFeatureFlags');
56
Scheduler = require('scheduler');
57
useState = React.useState;
58
useEffect = React.useEffect;
@@ -673,8 +671,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
671
});
672
673
describe('extra features implemented in user-space', () => {
676
- // The selector implementation uses the lazy ref initialization pattern
677
- // @gate !(enableUseRefAccessWarning && __DEV__)
674
it('memoized selectors are only called once per update', async () => {
675
const store = createExternalStore({a: 0, b: 0});
676
@@ -716,8 +712,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
712
expect(container.textContent).toEqual('A1');
713
});
714
719
- // The selector implementation uses the lazy ref initialization pattern
720
- // @gate !(enableUseRefAccessWarning && __DEV__)
715
it('Using isEqual to bailout', async () => {
716
const store = createExternalStore({a: 0, b: 0});
717
@@ -857,8 +851,6 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
851
expect(container.textContent).toEqual('UPDATED');
852
});
853
860
- // The selector implementation uses the lazy ref initialization pattern
861
- // @gate !(enableUseRefAccessWarning && __DEV__)
854
it('compares selection to rendered selection even if selector changes', async () => {
855
const store = createExternalStore({items: ['A', 'B']});
856
@@ -980,27 +972,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
972
973
if (__DEV__ && gate(flags => flags.enableUseSyncExternalStoreShim)) {
974
// In 17, the error is re-thrown in DEV.
983
- await expect(async () => {
984
- await expect(async () => {
985
- await act(() => {
986
- store.set({});
987
- });
988
- }).rejects.toThrow('Malformed state');
989
- }).toWarnDev(
990
- ReactFeatureFlags.enableUseRefAccessWarning
991
- ? ['Warning: App: Unsafe read of a mutable value during render.']
992
- : [],
993
- );
994
- } else {
975
await expect(async () => {
976
await act(() => {
977
store.set({});
978
});
999
- }).toWarnDev(
1000
- ReactFeatureFlags.enableUseRefAccessWarning
1001
- ? ['Warning: App: Unsafe read of a mutable value during render.']
1002
- : [],
1003
- );
979
+ }).rejects.toThrow('Malformed state');
980
+ } else {
981
+ await act(() => {
982
+ store.set({});
983
+ });
984
}
985
986
expect(container.textContent).toEqual('Malformed state');
@@ -1041,27 +1021,15 @@ describe('Shared useSyncExternalStore behavior (shim and built-in)', () => {
1021
1022
if (__DEV__ && gate(flags => flags.enableUseSyncExternalStoreShim)) {
1023
// In 17, the error is re-thrown in DEV.
1044
- await expect(async () => {
1045
- await expect(async () => {
1046
- await act(() => {
1047
- store.set({});
1048
- });
1049
- }).rejects.toThrow('Malformed state');
1050
- }).toWarnDev(
1051
- ReactFeatureFlags.enableUseRefAccessWarning
1052
- ? ['Warning: App: Unsafe read of a mutable value during render.']
1053
- : [],
1054
- );
1055
- } else {
1024
await expect(async () => {
1025
await act(() => {
1026
store.set({});
1027
});
1060
- }).toWarnDev(
1061
- ReactFeatureFlags.enableUseRefAccessWarning
1062
- ? ['Warning: App: Unsafe read of a mutable value during render.']
1063
- : [],
1064
- );
1028
+ }).rejects.toThrow('Malformed state');
1029
+ } else {
1030
+ await act(() => {
1031
+ store.set({});
1032
+ });
1033
}
1034
1035
expect(container.textContent).toEqual('Malformed state');