Skip to content

Commit 269dd6e

Browse files
authored
subtreeFlag warning: Fix legacy suspense false positive (#21388)
Legacy Suspense is weird. We intentionally commit a suspended fiber in an inconsistent state. If the fiber suspended before it mounted any effects, then the fiber won't have a PassiveStatic effect flag, which will trigger the "missing expected subtreeFlag" warning. To avoid the false positive, we'd need to mark fibers that commit in an incomplete state, somehow. For now I'll disable the warning in legacy mode, with the assumption that most of the bugs that would trigger it are either exclusive to concurrent mode or exist in both.
1 parent 3c21aa8 commit 269dd6e

File tree

3 files changed

+187
-2
lines changed

3 files changed

+187
-2
lines changed

packages/react-reconciler/src/ReactFiberHooks.new.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,13 @@ export function renderWithHooks<Props, SecondArg>(
480480
if (
481481
current !== null &&
482482
(current.flags & StaticMaskEffect) !==
483-
(workInProgress.flags & StaticMaskEffect)
483+
(workInProgress.flags & StaticMaskEffect) &&
484+
// Disable this warning in legacy mode, because legacy Suspense is weird
485+
// and creates false positives. To make this work in legacy mode, we'd
486+
// need to mark fibers that commit in an incomplete state, somehow. For
487+
// now I'll disable the warning that most of the bugs that would trigger
488+
// it are either exclusive to concurrent mode or exist in both.
489+
(current.mode & ConcurrentMode) !== NoMode
484490
) {
485491
console.error(
486492
'Internal React error: Expected static flag was missing. Please ' +

packages/react-reconciler/src/ReactFiberHooks.old.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,13 @@ export function renderWithHooks<Props, SecondArg>(
480480
if (
481481
current !== null &&
482482
(current.flags & StaticMaskEffect) !==
483-
(workInProgress.flags & StaticMaskEffect)
483+
(workInProgress.flags & StaticMaskEffect) &&
484+
// Disable this warning in legacy mode, because legacy Suspense is weird
485+
// and creates false positives. To make this work in legacy mode, we'd
486+
// need to mark fibers that commit in an incomplete state, somehow. For
487+
// now I'll disable the warning that most of the bugs that would trigger
488+
// it are either exclusive to concurrent mode or exist in both.
489+
(current.mode & ConcurrentMode) !== NoMode
484490
) {
485491
console.error(
486492
'Internal React error: Expected static flag was missing. Please ' +
Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
let React;
2+
let ReactNoop;
3+
let Scheduler;
4+
let Suspense;
5+
let useEffect;
6+
let getCacheForType;
7+
8+
let caches;
9+
let seededCache;
10+
11+
describe('ReactSuspenseWithNoopRenderer', () => {
12+
beforeEach(() => {
13+
jest.resetModules();
14+
15+
React = require('react');
16+
ReactNoop = require('react-noop-renderer');
17+
Scheduler = require('scheduler');
18+
Suspense = React.Suspense;
19+
useEffect = React.useEffect;
20+
21+
getCacheForType = React.unstable_getCacheForType;
22+
23+
caches = [];
24+
seededCache = null;
25+
});
26+
27+
function createTextCache() {
28+
if (seededCache !== null) {
29+
// Trick to seed a cache before it exists.
30+
// TODO: Need a built-in API to seed data before the initial render (i.e.
31+
// not a refresh because nothing has mounted yet).
32+
const cache = seededCache;
33+
seededCache = null;
34+
return cache;
35+
}
36+
37+
const data = new Map();
38+
const version = caches.length + 1;
39+
const cache = {
40+
version,
41+
data,
42+
resolve(text) {
43+
const record = data.get(text);
44+
if (record === undefined) {
45+
const newRecord = {
46+
status: 'resolved',
47+
value: text,
48+
};
49+
data.set(text, newRecord);
50+
} else if (record.status === 'pending') {
51+
const thenable = record.value;
52+
record.status = 'resolved';
53+
record.value = text;
54+
thenable.pings.forEach(t => t());
55+
}
56+
},
57+
reject(text, error) {
58+
const record = data.get(text);
59+
if (record === undefined) {
60+
const newRecord = {
61+
status: 'rejected',
62+
value: error,
63+
};
64+
data.set(text, newRecord);
65+
} else if (record.status === 'pending') {
66+
const thenable = record.value;
67+
record.status = 'rejected';
68+
record.value = error;
69+
thenable.pings.forEach(t => t());
70+
}
71+
},
72+
};
73+
caches.push(cache);
74+
return cache;
75+
}
76+
77+
function readText(text) {
78+
const textCache = getCacheForType(createTextCache);
79+
const record = textCache.data.get(text);
80+
if (record !== undefined) {
81+
switch (record.status) {
82+
case 'pending':
83+
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
84+
throw record.value;
85+
case 'rejected':
86+
Scheduler.unstable_yieldValue(`Error! [${text}]`);
87+
throw record.value;
88+
case 'resolved':
89+
return textCache.version;
90+
}
91+
} else {
92+
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
93+
94+
const thenable = {
95+
pings: [],
96+
then(resolve) {
97+
if (newRecord.status === 'pending') {
98+
thenable.pings.push(resolve);
99+
} else {
100+
Promise.resolve().then(() => resolve(newRecord.value));
101+
}
102+
},
103+
};
104+
105+
const newRecord = {
106+
status: 'pending',
107+
value: thenable,
108+
};
109+
textCache.data.set(text, newRecord);
110+
111+
throw thenable;
112+
}
113+
}
114+
115+
function resolveMostRecentTextCache(text) {
116+
if (caches.length === 0) {
117+
throw Error('Cache does not exist.');
118+
} else {
119+
// Resolve the most recently created cache. An older cache can by
120+
// resolved with `caches[index].resolve(text)`.
121+
caches[caches.length - 1].resolve(text);
122+
}
123+
}
124+
125+
const resolveText = resolveMostRecentTextCache;
126+
127+
// @gate experimental
128+
it('regression: false positive for legacy suspense', async () => {
129+
// Wrapping in memo because regular function components go through the
130+
// mountIndeterminateComponent path, which acts like there's no `current`
131+
// fiber even though there is. `memo` is not indeterminate, so it goes
132+
// through the update path.
133+
const Child = React.memo(({text}) => {
134+
// If text hasn't resolved, this will throw and exit before the passive
135+
// static effect flag is added by the useEffect call below.
136+
readText(text);
137+
138+
useEffect(() => {
139+
Scheduler.unstable_yieldValue('Effect');
140+
}, []);
141+
142+
Scheduler.unstable_yieldValue(text);
143+
return text;
144+
});
145+
146+
function App() {
147+
return (
148+
<Suspense fallback="Loading...">
149+
<Child text="Async" />
150+
</Suspense>
151+
);
152+
}
153+
154+
const root = ReactNoop.createLegacyRoot(null);
155+
156+
// On initial mount, the suspended component is committed in an incomplete
157+
// state, without a passive static effect flag.
158+
await ReactNoop.act(async () => {
159+
root.render(<App />);
160+
});
161+
expect(Scheduler).toHaveYielded(['Suspend! [Async]']);
162+
expect(root).toMatchRenderedOutput('Loading...');
163+
164+
// When the promise resolves, a passive static effect flag is added. In the
165+
// regression, the "missing expected static flag" would fire, because the
166+
// previous fiber did not have one.
167+
await ReactNoop.act(async () => {
168+
resolveText('Async');
169+
});
170+
expect(Scheduler).toHaveYielded(['Async', 'Effect']);
171+
expect(root).toMatchRenderedOutput('Async');
172+
});
173+
});

0 commit comments

Comments
 (0)