Skip to content

Commit

Permalink
Fix for #18657 (#18663)
Browse files Browse the repository at this point in the history
* Failing test for #18657

* Remove incorrect priority check

I think this was just poor factoring on my part in #18411. Honestly it
doesn't make much sense to me, but my best guess is that I must have
thought that when `baseTime > currentChildExpirationTime`, the function
would fall through to the
`currentChildExpirationTime < renderExpirationTime` branch below.

Really I think just made an oopsie.

Regardless, this logic is galaxy brainéd. A goal of the Lanes refactor
I'm working on is to make these types of checks -- is there remaining
work in this tree? -- a lot easier to think about. Hopefully.
  • Loading branch information
acdlite authored Apr 17, 2020
1 parent e7163a9 commit cfefc81
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 10 deletions.
6 changes: 1 addition & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1681,11 +1681,7 @@ function getRemainingWorkInPrimaryTree(
// This boundary already timed out. Check if this render includes the level
// that previously suspended.
const baseTime = currentSuspenseState.baseTime;
if (
baseTime !== NoWork &&
baseTime < renderExpirationTime &&
baseTime > currentChildExpirationTime
) {
if (baseTime !== NoWork && baseTime < renderExpirationTime) {
// There's pending work at a lower level that might now be unblocked.
return baseTime;
}
Expand Down
6 changes: 1 addition & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1681,11 +1681,7 @@ function getRemainingWorkInPrimaryTree(
// This boundary already timed out. Check if this render includes the level
// that previously suspended.
const baseTime = currentSuspenseState.baseTime;
if (
baseTime !== NoWork &&
baseTime < renderExpirationTime &&
baseTime > currentChildExpirationTime
) {
if (baseTime !== NoWork && baseTime < renderExpirationTime) {
// There's pending work at a lower level that might now be unblocked.
return baseTime;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3869,4 +3869,53 @@ describe('ReactSuspenseWithNoopRenderer', () => {
expect(root).toMatchRenderedOutput(<span prop="b" />);
});
});

it('regression: #18657', async () => {
const {useState} = React;

let setText;
function App() {
const [text, _setText] = useState('A');
setText = _setText;
return <AsyncText text={text} />;
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
await resolveText('A');
root.render(
<Suspense fallback={<Text text="Loading..." />}>
<App />
</Suspense>,
);
});
expect(Scheduler).toHaveYielded(['A']);
expect(root).toMatchRenderedOutput(<span prop="A" />);

await ReactNoop.act(async () => {
setText('B');
Scheduler.unstable_runWithPriority(
Scheduler.unstable_IdlePriority,
() => {
setText('B');
},
);
// Suspend the first update. The second update doesn't run because it has
// Idle priority.
expect(Scheduler).toFlushAndYield(['Suspend! [B]', 'Loading...']);

// Commit the fallback. Now we'll try working on Idle.
jest.runAllTimers();

// It also suspends.
expect(Scheduler).toFlushAndYield(['Suspend! [B]']);
});

await ReactNoop.act(async () => {
setText('B');
await resolveText('B');
});
expect(Scheduler).toHaveYielded(['Promise resolved [B]', 'B']);
expect(root).toMatchRenderedOutput(<span prop="B" />);
});
});

0 comments on commit cfefc81

Please sign in to comment.