-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Reset lastEffect when resuming SuspenseList #18412
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2300,4 +2300,189 @@ describe('ReactSuspenseList', () => { | |
// remount. | ||
expect(previousInst).toBe(setAsyncB); | ||
}); | ||
|
||
it('is able to re-suspend the last rows during an update with hidden', async () => { | ||
let AsyncB = createAsyncText('B'); | ||
|
||
let setAsyncB; | ||
|
||
function B() { | ||
let [shouldBeAsync, setAsync] = React.useState(false); | ||
setAsyncB = setAsync; | ||
|
||
return shouldBeAsync ? ( | ||
<Suspense fallback={<Text text="Loading B" />}> | ||
<AsyncB /> | ||
</Suspense> | ||
) : ( | ||
<Text text="Sync B" /> | ||
); | ||
} | ||
|
||
function Foo({updateList}) { | ||
return ( | ||
<SuspenseList revealOrder="forwards" tail="hidden"> | ||
<Suspense key="A" fallback={<Text text="Loading A" />}> | ||
<Text text="A" /> | ||
</Suspense> | ||
<B key="B" updateList={updateList} /> | ||
</SuspenseList> | ||
); | ||
} | ||
|
||
ReactNoop.render(<Foo />); | ||
|
||
expect(Scheduler).toFlushAndYield(['A', 'Sync B']); | ||
|
||
expect(ReactNoop).toMatchRenderedOutput( | ||
<> | ||
<span>A</span> | ||
<span>Sync B</span> | ||
</>, | ||
); | ||
|
||
let previousInst = setAsyncB; | ||
|
||
// During an update we suspend on B. | ||
ReactNoop.act(() => setAsyncB(true)); | ||
|
||
expect(Scheduler).toHaveYielded([ | ||
'Suspend! [B]', | ||
'Loading B', | ||
// The second pass is the "force hide" pass | ||
'Loading B', | ||
]); | ||
|
||
expect(ReactNoop).toMatchRenderedOutput( | ||
<> | ||
<span>A</span> | ||
<span>Loading B</span> | ||
</>, | ||
); | ||
|
||
// Before we resolve we'll rerender the whole list. | ||
// This should leave the tree intact. | ||
ReactNoop.act(() => ReactNoop.render(<Foo updateList={true} />)); | ||
|
||
expect(Scheduler).toHaveYielded(['A', 'Suspend! [B]', 'Loading B']); | ||
|
||
expect(ReactNoop).toMatchRenderedOutput( | ||
<> | ||
<span>A</span> | ||
<span>Loading B</span> | ||
</>, | ||
); | ||
|
||
await AsyncB.resolve(); | ||
|
||
expect(Scheduler).toFlushAndYield(['B']); | ||
|
||
expect(ReactNoop).toMatchRenderedOutput( | ||
<> | ||
<span>A</span> | ||
<span>B</span> | ||
</>, | ||
); | ||
|
||
// This should be the same instance. I.e. it didn't | ||
// remount. | ||
expect(previousInst).toBe(setAsyncB); | ||
}); | ||
|
||
it('is able to interrupt a partially rendered tree and continue later', async () => { | ||
let AsyncA = createAsyncText('A'); | ||
|
||
let updateLowPri; | ||
let updateHighPri; | ||
|
||
function Bar() { | ||
let [highPriState, setHighPriState] = React.useState(false); | ||
updateHighPri = setHighPriState; | ||
return highPriState ? <AsyncA /> : null; | ||
} | ||
|
||
function Foo() { | ||
let [lowPriState, setLowPriState] = React.useState(false); | ||
updateLowPri = setLowPriState; | ||
return ( | ||
<SuspenseList revealOrder="forwards" tail="hidden"> | ||
<Suspense key="A" fallback={<Text text="Loading A" />}> | ||
<Bar /> | ||
</Suspense> | ||
{lowPriState ? <Text text="B" /> : null} | ||
{lowPriState ? <Text text="C" /> : null} | ||
{lowPriState ? <Text text="D" /> : null} | ||
</SuspenseList> | ||
); | ||
} | ||
|
||
ReactNoop.render(<Foo />); | ||
|
||
expect(Scheduler).toFlushAndYield([]); | ||
|
||
expect(ReactNoop).toMatchRenderedOutput(null); | ||
|
||
ReactNoop.act(() => { | ||
// Add a few items at the end. | ||
updateLowPri(true); | ||
|
||
// Flush partially through. | ||
expect(Scheduler).toFlushAndYieldThrough(['B', 'C']); | ||
|
||
// Schedule another update at higher priority. | ||
Scheduler.unstable_runWithPriority( | ||
Scheduler.unstable_UserBlockingPriority, | ||
() => updateHighPri(true), | ||
); | ||
|
||
// That will intercept the previous render. | ||
}); | ||
|
||
jest.runAllTimers(); | ||
|
||
expect(Scheduler).toHaveYielded( | ||
__DEV__ | ||
? [ | ||
// First attempt at high pri. | ||
'Suspend! [A]', | ||
'Loading A', | ||
// Re-render at forced. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "forced" mean here? Which branch does it correspond to? Explain like I'm five. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SuspenseList sets the suspense context that during render “forces” the suspense boundary into fallback state. In this case it’s only one boundary so it doesn’t need to but we don’t know. If there was another boundary in the row we would have to rerender that one so that’s the second pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ohh that makes sense! I didn’t realize it works this way. |
||
'Suspend! [A]', | ||
'Loading A', | ||
// We auto-commit this on DEV. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? "Flush fallbacks" flag? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea |
||
// Try again on low-pri. | ||
'Suspend! [A]', | ||
'Loading A', | ||
] | ||
: [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is verifying the same logs appear three or four times. But how do we know the comments are actually telling the truth? Can we include the boolean values in logs so that we know we're actually rendering at the priority suggested by comments? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’d rather just delete this assertion. It’s unrelated. It’s just that Scheduler expects force me to assert. This is super suboptimal so it’ll change by heuristics a lot. |
||
// First attempt at high pri. | ||
'Suspend! [A]', | ||
'Loading A', | ||
// Re-render at forced. | ||
'Suspend! [A]', | ||
'Loading A', | ||
// We didn't commit so retry at low-pri. | ||
'Suspend! [A]', | ||
'Loading A', | ||
// Re-render at forced. | ||
'Suspend! [A]', | ||
'Loading A', | ||
], | ||
); | ||
|
||
expect(ReactNoop).toMatchRenderedOutput(<span>Loading A</span>); | ||
|
||
await AsyncA.resolve(); | ||
|
||
expect(Scheduler).toFlushAndYield(['A', 'B', 'C', 'D']); | ||
|
||
expect(ReactNoop).toMatchRenderedOutput( | ||
<> | ||
<span>A</span> | ||
<span>B</span> | ||
<span>C</span> | ||
<span>D</span> | ||
</>, | ||
); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interrupt? :-)