Skip to content

Strict mode and default mode should have same Suspense semantics #13882

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.

Sign up for GitHub

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

Merged
acdlite merged 1 commit into from
Oct 18, 2018
Merged

Strict mode and default mode should have same Suspense semantics #13882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions packages/react-reconciler/src/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,7 @@ import {
computeAsyncExpiration,
computeInteractiveExpiration,
} from './ReactFiberExpirationTime';
import {
ConcurrentMode,
ProfileMode,
NoContext,
StrictMode,
} from './ReactTypeOfMode';
import {ConcurrentMode, ProfileMode, NoContext} from './ReactTypeOfMode';
import {enqueueUpdate, resetCurrentlyProcessingQueue} from './ReactUpdateQueue';
import {createCapturedValue} from './ReactCapturedValue';
import {
Expand Down Expand Up @@ -1602,10 +1597,10 @@ function retrySuspendedRoot(
}

scheduleWorkToRoot(boundaryFiber, retryTime);
if ((boundaryFiber.mode & StrictMode) === NoContext) {
// Outside of strict mode, we must schedule an update on the source fiber,
// too, since it already committed in an inconsistent state and therefore
// does not have any pending work.
if ((boundaryFiber.mode & ConcurrentMode) === NoContext) {
// Outside of concurrent mode, we must schedule an update on the source
// fiber, too, since it already committed in an inconsistent state and
// therefore does not have any pending work.
scheduleWorkToRoot(sourceFiber, retryTime);
}

Expand Down
18 changes: 9 additions & 9 deletions packages/react-reconciler/src/ReactFiberUnwindWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {
LifecycleEffectMask,
} from 'shared/ReactSideEffectTags';
import {enableSchedulerTracing} from 'shared/ReactFeatureFlags';
import {StrictMode, ConcurrentMode} from './ReactTypeOfMode';
import {ConcurrentMode} from './ReactTypeOfMode';

import {createCapturedValue} from './ReactCapturedValue';
import {
Expand Down Expand Up @@ -230,15 +230,15 @@ function throwException(
}
thenable.then(onResolveOrReject, onResolveOrReject);

// If the boundary is outside of strict mode, we should *not* suspend
// the commit. Pretend as if the suspended component rendered null and
// keep rendering. In the commit phase, we'll schedule a subsequent
// synchronous update to re-render the Suspense.
// If the boundary is outside of concurrent mode, we should *not*
// suspend the commit. Pretend as if the suspended component rendered
// null and keep rendering. In the commit phase, we'll schedule a
// subsequent synchronous update to re-render the Suspense.
//
// Note: It doesn't matter whether the component that suspended was
// inside a strict mode tree. If the Suspense is outside of it, we
// inside a concurrent mode tree. If the Suspense is outside of it, we
// should *not* suspend the commit.
if ((workInProgress.mode & StrictMode) === NoEffect) {
if ((workInProgress.mode & ConcurrentMode) === NoEffect) {
workInProgress.effectTag |= CallbackEffect;

// Unmount the source fiber's children
Expand Down Expand Up @@ -274,8 +274,8 @@ function throwException(
return;
}

// Confirmed that the boundary is in a strict mode tree. Continue with
// the normal suspend path.
// Confirmed that the boundary is in a concurrent mode tree. Continue
// with the normal suspend path.

let absoluteTimeoutMs;
if (earliestTimeoutMs === -1) {
Expand Down
105 changes: 54 additions & 51 deletions packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,7 +1023,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {

it(
'continues rendering asynchronously even if a promise is captured by ' +
'a sync boundary (strict)',
'a sync boundary (default mode)',
async () => {
class UpdatingText extends React.Component {
state = {text: this.props.initialText};
Expand All @@ -1036,7 +1036,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
const text2 = React.createRef(null);
function App() {
return (
<StrictMode>
<Fragment>
<Suspense
maxDuration={1000}
fallback={<Text text="Loading..." />}>
Expand All @@ -1063,7 +1063,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
)}
</UpdatingText>
</ConcurrentMode>
</StrictMode>
</Fragment>
);
}

Expand All @@ -1076,25 +1076,26 @@ describe('ReactSuspenseWithNoopRenderer', () => {
'Before',
'Suspend! [Async: 1]',
'After',
'Loading...',
'Before',
'Sync: 1',
'After',
'Did mount',
// The placeholder is rendered in a subsequent commit
'Loading...',
'Promise resolved [Async: 1]',
'Before',
'Async: 1',
'After',
]);
expect(ReactNoop.getChildren()).toEqual([
span('Before'),
span('Async: 1'),
span('After'),

span('Before'),
span('Sync: 1'),
span('After'),
]);
expect(ReactNoop.getChildrenAsJSX()).toEqual(
<React.Fragment>
<span prop="Before" />
<span prop="Async: 1" />
<span prop="After" />

<span prop="Before" />
<span prop="Sync: 1" />
<span prop="After" />
</React.Fragment>,
);

// Update. This starts out asynchronously.
text1.current.setState({text: 'Async: 2'}, () =>
Expand All @@ -1105,59 +1106,61 @@ describe('ReactSuspenseWithNoopRenderer', () => {
);

// Start rendering asynchronously
ReactNoop.flushThrough([
'Before',
ReactNoop.flushThrough(['Before']);

// Now render the next child, which suspends
expect(ReactNoop.flushNextYield()).toEqual([
// This child suspends
'Suspend! [Async: 2]',
// But we can still render the rest of the async tree asynchronously
'After',
]);

// Suspend during an async render.
expect(ReactNoop.flushNextYield()).toEqual(['Loading...']);
expect(ReactNoop.flush()).toEqual(['Before', 'Sync: 2', 'After']);
// Commit was suspended.
expect(ReactNoop.getChildren()).toEqual([
span('Before'),
span('Async: 1'),
span('After'),

span('Before'),
span('Sync: 1'),
span('After'),
]);

// When the placeholder is pinged, the boundary re-
// renders asynchronously.
ReactNoop.expire(100);
await advanceTimers(100);
expect(ReactNoop.flush()).toEqual([
'Promise resolved [Async: 2]',
'Before',
'Async: 2',
'After',
'Before',
'Sync: 2',
'After',
'Update 1 did commit',
'Update 2 did commit',

// Switch to the placeholder in a subsequent commit
'Loading...',
]);
expect(ReactNoop.getChildrenAsJSX()).toEqual(
<React.Fragment>
<span hidden={true} prop="Before" />
<span hidden={true} prop="After" />
<span prop="Loading..." />

expect(ReactNoop.getChildren()).toEqual([
span('Before'),
span('Async: 2'),
span('After'),
<span prop="Before" />
<span prop="Sync: 2" />
<span prop="After" />
</React.Fragment>,
);

span('Before'),
span('Sync: 2'),
span('After'),
// When the placeholder is pinged, the boundary must be re-rendered
// synchronously.
await advanceTimers(100);
expect(ReactNoop.clearYields()).toEqual([
'Promise resolved [Async: 2]',
'Async: 2',
]);

expect(ReactNoop.getChildrenAsJSX()).toEqual(
<React.Fragment>
<span prop="Before" />
<span prop="Async: 2" />
<span prop="After" />

<span prop="Before" />
<span prop="Sync: 2" />
<span prop="After" />
</React.Fragment>,
);
},
);

it(
'continues rendering asynchronously even if a promise is captured by ' +
'a sync boundary (loose)',
'a sync boundary (strict, non-concurrent)',
async () => {
class UpdatingText extends React.Component {
state = {text: this.props.initialText};
Expand All @@ -1170,7 +1173,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
const text2 = React.createRef(null);
function App() {
return (
<Fragment>
<StrictMode>
<Suspense
maxDuration={1000}
fallback={<Text text="Loading..." />}>
Expand All @@ -1197,7 +1200,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
)}
</UpdatingText>
</ConcurrentMode>
</Fragment>
</StrictMode>
);
}

Expand Down