Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subtreeFlag warning: Fix legacy suspense false positive #21388

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.new.js
Expand Up @@ -480,7 +480,13 @@ export function renderWithHooks<Props, SecondArg>(
if (
current !== null &&
(current.flags & StaticMaskEffect) !==
(workInProgress.flags & StaticMaskEffect)
(workInProgress.flags & StaticMaskEffect) &&
// Disable this warning in legacy mode, because legacy Suspense is weird
// and creates false positives. To make this work in legacy mode, we'd
// need to mark fibers that commit in an incomplete state, somehow. For
// now I'll disable the warning that most of the bugs that would trigger
// it are either exclusive to concurrent mode or exist in both.
(current.mode & ConcurrentMode) !== NoMode
) {
console.error(
'Internal React error: Expected static flag was missing. Please ' +
Expand Down
8 changes: 7 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.old.js
Expand Up @@ -480,7 +480,13 @@ export function renderWithHooks<Props, SecondArg>(
if (
current !== null &&
(current.flags & StaticMaskEffect) !==
(workInProgress.flags & StaticMaskEffect)
(workInProgress.flags & StaticMaskEffect) &&
// Disable this warning in legacy mode, because legacy Suspense is weird
// and creates false positives. To make this work in legacy mode, we'd
// need to mark fibers that commit in an incomplete state, somehow. For
// now I'll disable the warning that most of the bugs that would trigger
// it are either exclusive to concurrent mode or exist in both.
(current.mode & ConcurrentMode) !== NoMode
) {
console.error(
'Internal React error: Expected static flag was missing. Please ' +
Expand Down
@@ -0,0 +1,173 @@
let React;
let ReactNoop;
let Scheduler;
let Suspense;
let useEffect;
let getCacheForType;

let caches;
let seededCache;

describe('ReactSuspenseWithNoopRenderer', () => {
beforeEach(() => {
jest.resetModules();

React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
Suspense = React.Suspense;
useEffect = React.useEffect;

getCacheForType = React.unstable_getCacheForType;

caches = [];
seededCache = null;
});

function createTextCache() {
if (seededCache !== null) {
// Trick to seed a cache before it exists.
// TODO: Need a built-in API to seed data before the initial render (i.e.
// not a refresh because nothing has mounted yet).
const cache = seededCache;
seededCache = null;
return cache;
}

const data = new Map();
const version = caches.length + 1;
const cache = {
version,
data,
resolve(text) {
const record = data.get(text);
if (record === undefined) {
const newRecord = {
status: 'resolved',
value: text,
};
data.set(text, newRecord);
} else if (record.status === 'pending') {
const thenable = record.value;
record.status = 'resolved';
record.value = text;
thenable.pings.forEach(t => t());
}
},
reject(text, error) {
const record = data.get(text);
if (record === undefined) {
const newRecord = {
status: 'rejected',
value: error,
};
data.set(text, newRecord);
} else if (record.status === 'pending') {
const thenable = record.value;
record.status = 'rejected';
record.value = error;
thenable.pings.forEach(t => t());
}
},
};
caches.push(cache);
return cache;
}

function readText(text) {
const textCache = getCacheForType(createTextCache);
const record = textCache.data.get(text);
if (record !== undefined) {
switch (record.status) {
case 'pending':
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
throw record.value;
case 'rejected':
Scheduler.unstable_yieldValue(`Error! [${text}]`);
throw record.value;
case 'resolved':
return textCache.version;
}
} else {
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);

const thenable = {
pings: [],
then(resolve) {
if (newRecord.status === 'pending') {
thenable.pings.push(resolve);
} else {
Promise.resolve().then(() => resolve(newRecord.value));
}
},
};

const newRecord = {
status: 'pending',
value: thenable,
};
textCache.data.set(text, newRecord);

throw thenable;
}
}

function resolveMostRecentTextCache(text) {
if (caches.length === 0) {
throw Error('Cache does not exist.');
} else {
// Resolve the most recently created cache. An older cache can by
// resolved with `caches[index].resolve(text)`.
caches[caches.length - 1].resolve(text);
}
}

const resolveText = resolveMostRecentTextCache;

// @gate experimental
it('regression: false positive for legacy suspense', async () => {
// Wrapping in memo because regular function components go through the
// mountIndeterminateComponent path, which acts like there's no `current`
// fiber even though there is. `memo` is not indeterminate, so it goes
// through the update path.
const Child = React.memo(({text}) => {
// If text hasn't resolved, this will throw and exit before the passive
// static effect flag is added by the useEffect call below.
readText(text);

useEffect(() => {
Scheduler.unstable_yieldValue('Effect');
}, []);

Scheduler.unstable_yieldValue(text);
return text;
});

function App() {
return (
<Suspense fallback="Loading...">
<Child text="Async" />
</Suspense>
);
}

const root = ReactNoop.createLegacyRoot(null);

// On initial mount, the suspended component is committed in an incomplete
// state, without a passive static effect flag.
await ReactNoop.act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['Suspend! [Async]']);
expect(root).toMatchRenderedOutput('Loading...');

// When the promise resolves, a passive static effect flag is added. In the
// regression, the "missing expected static flag" would fire, because the
// previous fiber did not have one.
await ReactNoop.act(async () => {
resolveText('Async');
});
expect(Scheduler).toHaveYielded(['Async', 'Effect']);
expect(root).toMatchRenderedOutput('Async');
});
});