Account for another DevTools + Fast Refresh edge case (#21523)
DevTools now 'untrack' Fibers (cleans up the ID-to-Fiber mapping) after a slight delay in order to support a Fast Refresh edge case: 1. Component type is updated and Fast Refresh schedules an update+remount. 2. flushPendingErrorsAndWarningsAfterDelay() runs, sees the old Fiber is no longer mounted (it's been disconnected by Fast Refresh), and calls untrackFiberID() to clear it from the Map. 3. React flushes pending passive effects before it runs the next render, which logs an error or warning, which causes a new ID to be generated for this Fiber. 4. DevTools now tries to unmount the old Component with the new ID. The underlying problem here is the premature clearing of the Fiber ID, but DevTools has no way to detect that a given Fiber has been scheduled for Fast Refresh. (The '_debugNeedsRemount' flag won't necessarily be set.) The best we can do is to delay untracking by a small amount, and give React time to process the Fast Refresh delay.
Brian Vaughn committed
May 18, 2021 at 19:44 UTC
d6604ac0318f2ee139492d8dce64d6427e921658
3 files changed
+98
-28
packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js
+15
-11
@@ -188,13 +188,13 @@ describe('Fast Refresh', () => {
188
});
189
190
it('should not break when there are warnings in between patching', () => {
191
- withErrorsOrWarningsIgnored(['Expected warning during render'], () => {
191
+ withErrorsOrWarningsIgnored(['Expected:'], () => {
192
render(`
193
const {useState} = React;
194
195
export default function Component() {
196
const [state, setState] = useState(1);
197
- console.warn("Expected warning during render");
197
+ console.warn("Expected: warning during render");
198
return null;
199
}
200
`);
@@ -205,13 +205,13 @@ describe('Fast Refresh', () => {
205
<Component> ⚠
206
`);
207
208
- withErrorsOrWarningsIgnored(['Expected warning during render'], () => {
208
+ withErrorsOrWarningsIgnored(['Expected:'], () => {
209
patch(`
210
const {useEffect, useState} = React;
211
212
export default function Component() {
213
const [state, setState] = useState(1);
214
- console.warn("Expected warning during render");
214
+ console.warn("Expected: warning during render");
215
return null;
216
}
217
`);
@@ -222,31 +222,33 @@ describe('Fast Refresh', () => {
222
<Component> ⚠
223
`);
224
225
- withErrorsOrWarningsIgnored(['Expected warning during render'], () => {
225
+ withErrorsOrWarningsIgnored(['Expected:'], () => {
226
patch(`
227
const {useEffect, useState} = React;
228
229
export default function Component() {
230
const [state, setState] = useState(1);
231
- useEffect(() => {});
232
- console.warn("Expected warning during render");
231
+ useEffect(() => {
232
+ console.error("Expected: error during effect");
233
+ });
234
+ console.warn("Expected: warning during render");
235
return null;
236
}
237
`);
238
});
239
expect(store).toMatchInlineSnapshot(`
238
- ✕ 0, ⚠ 1
240
+ ✕ 1, ⚠ 1
241
[root]
240
- <Component> ⚠
242
+ <Component> ✕⚠
243
`);
244
243
- withErrorsOrWarningsIgnored(['Expected warning during render'], () => {
245
+ withErrorsOrWarningsIgnored(['Expected:'], () => {
246
patch(`
247
const {useEffect, useState} = React;
248
249
export default function Component() {
250
const [state, setState] = useState(1);
249
- console.warn("Expected warning during render");
251
+ console.warn("Expected: warning during render");
252
return null;
253
}
254
`);
@@ -257,4 +259,6 @@ describe('Fast Refresh', () => {
259
<Component> ⚠
260
`);
261
});
262
+
263
+ // TODO (bvaughn) Write a test that checks in between the steps of patch
264
});
packages/react-devtools-shared/src/backend/renderer.js
+82
-16
@@ -717,7 +717,7 @@ export function attach(
717
? getFiberIDUnsafe(parentFiber) || '<no-id>'
718
: '';
719
720
- console.log(
720
+ console.groupCollapsed(
721
`[renderer] %c${name} %c${displayName} (${maybeID}) %c${
722
parentFiber ? `${parentDisplayName} (${maybeParentID})` : ''
723
} %c${extraString}`,
@@ -726,6 +726,13 @@ export function attach(
726
'color: purple;',
727
'color: black;',
728
);
729
+ console.log(
730
+ new Error().stack
731
+ .split('\n')
732
+ .slice(1)
733
+ .join('\n'),
734
+ );
735
+ console.groupEnd();
736
}
737
};
738
@@ -996,7 +1003,9 @@ export function attach(
1003
}
1004
}
1005
1006
+ let didGenerateID = false;
1007
if (id === null) {
1008
+ didGenerateID = true;
1009
id = getUID();
1010
}
1011
@@ -1019,6 +1028,17 @@ export function attach(
1028
}
1029
}
1030
1031
+ if (__DEBUG__) {
1032
+ if (didGenerateID) {
1033
+ debug(
1034
+ 'getOrGenerateFiberID()',
1035
+ fiber,
1036
+ fiber.return,
1037
+ 'Generated a new UID',
1038
+ );
1039
+ }
1040
+ }
1041
+
1042
return refinedID;
1043
}
1044
@@ -1050,19 +1070,61 @@ export function attach(
1070
// Removes a Fiber (and its alternate) from the Maps used to track their id.
1071
// This method should always be called when a Fiber is unmounting.
1072
function untrackFiberID(fiber: Fiber) {
1053
- const fiberID = getFiberIDUnsafe(fiber);
1054
- if (fiberID !== null) {
1055
- idToArbitraryFiberMap.delete(fiberID);
1073
+ if (__DEBUG__) {
1074
+ debug('untrackFiberID()', fiber, fiber.return, 'schedule after delay');
1075
}
1076
1058
- fiberToIDMap.delete(fiber);
1077
+ // Untrack Fibers after a slight delay in order to support a Fast Refresh edge case:
1078
+ // 1. Component type is updated and Fast Refresh schedules an update+remount.
1079
+ // 2. flushPendingErrorsAndWarningsAfterDelay() runs, sees the old Fiber is no longer mounted
1080
+ // (it's been disconnected by Fast Refresh), and calls untrackFiberID() to clear it from the Map.
1081
+ // 3. React flushes pending passive effects before it runs the next render,
1082
+ // which logs an error or warning, which causes a new ID to be generated for this Fiber.
1083
+ // 4. DevTools now tries to unmount the old Component with the new ID.
1084
+ //
1085
+ // The underlying problem here is the premature clearing of the Fiber ID,
1086
+ // but DevTools has no way to detect that a given Fiber has been scheduled for Fast Refresh.
1087
+ // (The "_debugNeedsRemount" flag won't necessarily be set.)
1088
+ //
1089
+ // The best we can do is to delay untracking by a small amount,
1090
+ // and give React time to process the Fast Refresh delay.
1091
1060
- const {alternate} = fiber;
1061
- if (alternate !== null) {
1062
- fiberToIDMap.delete(alternate);
1092
+ untrackFibersSet.add(fiber);
1093
+
1094
+ if (untrackFibersTimeoutID === null) {
1095
+ untrackFibersTimeoutID = setTimeout(untrackFibers, 1000);
1096
}
1097
}
1098
1099
+ const untrackFibersSet: Set<Fiber> = new Set();
1100
+ let untrackFibersTimeoutID: TimeoutID | null = null;
1101
+
1102
+ function untrackFibers() {
1103
+ if (untrackFibersTimeoutID !== null) {
1104
+ clearTimeout(untrackFibersTimeoutID);
1105
+ untrackFibersTimeoutID = null;
1106
+ }
1107
+
1108
+ untrackFibersSet.forEach(fiber => {
1109
+ const fiberID = getFiberIDUnsafe(fiber);
1110
+ if (fiberID !== null) {
1111
+ idToArbitraryFiberMap.delete(fiberID);
1112
+
1113
+ // Also clear any errors/warnings associated with this fiber.
1114
+ clearErrorsForFiberID(fiberID);
1115
+ clearWarningsForFiberID(fiberID);
1116
+ }
1117
+
1118
+ fiberToIDMap.delete(fiber);
1119
+
1120
+ const {alternate} = fiber;
1121
+ if (alternate !== null) {
1122
+ fiberToIDMap.delete(alternate);
1123
+ }
1124
+ });
1125
+ untrackFibersSet.clear();
1126
+ }
1127
+
1128
function getChangeDescription(
1129
prevFiber: Fiber | null,
1130
nextFiber: Fiber,
@@ -1610,13 +1672,13 @@ export function attach(
1672
}
1673
1674
function recordMount(fiber: Fiber, parentFiber: Fiber | null) {
1675
+ const isRoot = fiber.tag === HostRoot;
1676
+ const id = getOrGenerateFiberID(fiber);
1677
+
1678
if (__DEBUG__) {
1679
debug('recordMount()', fiber, parentFiber);
1680
}
1681
1617
- const isRoot = fiber.tag === HostRoot;
1618
- const id = getOrGenerateFiberID(fiber);
1619
-
1682
const hasOwnerMetadata = fiber.hasOwnProperty('_debugOwner');
1683
const isProfilingSupported = fiber.hasOwnProperty('treeBaseDuration');
1684
@@ -1748,6 +1810,9 @@ export function attach(
1810
// This reduces the chance of stack overflow for wide trees (e.g. lists with many items).
1811
let fiber: Fiber | null = firstChild;
1812
while (fiber !== null) {
1813
+ // Generate an ID even for filtered Fibers, in case it's needed later (e.g. for Profiling).
1814
+ getOrGenerateFiberID(fiber);
1815
+
1816
if (__DEBUG__) {
1817
debug('mountFiberRecursively()', fiber, parentFiber);
1818
}
@@ -1761,9 +1826,6 @@ export function attach(
1826
const shouldIncludeInTree = !shouldFilterFiber(fiber);
1827
if (shouldIncludeInTree) {
1828
recordMount(fiber, parentFiber);
1764
- } else {
1765
- // Generate an ID even for filtered Fibers, in case it's needed later (e.g. for Profiling).
1766
- getOrGenerateFiberID(fiber);
1829
}
1830
1831
if (traceUpdatesEnabled) {
@@ -2008,12 +2070,12 @@ export function attach(
2070
parentFiber: Fiber | null,
2071
traceNearestHostComponentUpdate: boolean,
2072
): boolean {
2073
+ const id = getOrGenerateFiberID(nextFiber);
2074
+
2075
if (__DEBUG__) {
2076
debug('updateFiberRecursively()', nextFiber, parentFiber);
2077
}
2078
2015
- const id = getOrGenerateFiberID(nextFiber);
2016
-
2079
if (traceUpdatesEnabled) {
2080
const elementType = getElementTypeForFiber(nextFiber);
2081
if (traceNearestHostComponentUpdate) {
@@ -2322,6 +2384,10 @@ export function attach(
2384
const current = root.current;
2385
const alternate = current.alternate;
2386
2387
+ // Flush any pending Fibers that we are untracking before processing the new commit.
2388
+ // If we don't do this, we might end up double-deleting Fibers in some cases (like Legacy Suspense).
2389
+ untrackFibers();
2390
+
2391
currentRootID = getOrGenerateFiberID(current);
2392
2393
// Before the traversals, remember to start tracking
packages/react-devtools/CHANGELOG.md
+1
-1
@@ -14,7 +14,7 @@
14
* Updated `react` and `react-dom` API imports in preparation for upcoming stable release ([bvaughn](https://github.com/bvaughn) in [#21488](https://github.com/facebook/react/pull/21488))
15
16
#### Bugfix
17
-* Reload all roots after Fast Refresh force-remount (to avoid corrupted Store state) ([bvaughn](https://github.com/bvaughn) in [#21516](https://github.com/facebook/react/pull/21516))
17
+* Reload all roots after Fast Refresh force-remount (to avoid corrupted Store state) ([bvaughn](https://github.com/bvaughn) in [#21516](https://github.com/facebook/react/pull/21516) and [#21523](https://github.com/facebook/react/pull/21523))
18
* Errors thrown by Store can be dismissed so DevTools remain usable in many cases ([bvaughn](https://github.com/bvaughn) in [#21520](https://github.com/facebook/react/pull/21520))
19
* Fixed string concatenation problem when a `Symbol` was logged to `console.error` or `console.warn` ([bvaughn](https://github.com/bvaughn) in [#21521](https://github.com/facebook/react/pull/21521))
20
* DevTools: Fixed version range NPM syntax