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

Reset lastEffect when resuming SuspenseList #18412

Merged
merged 1 commit into from
Mar 29, 2020

Conversation

sebmarkbage
Copy link
Collaborator

We store an effect pointer so we can backtrack in the effect list in some cases. This is a stateful variable. If we interrupt a render we need to reset it.

This field was added after the optimization was added and I didn't remember to reset it here.

Otherwise we end up not resetting the firstEffect so it points to a stale list. As a result children don't end up inserted like we think they were. Then we try to remove them it errors.

It would be nicer to just get rid of the effect list and use the tree for effects instead. Maybe we still need something for deletions tho.

@facebook-github-bot facebook-github-bot added React Core Team Opened by a member of the React Core Team CLA Signed labels Mar 28, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 28, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a86b002:

Sandbox Source
cool-ellis-c1z3o Configuration

@sizebot
Copy link

sizebot commented Mar 28, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against a86b002

@sizebot
Copy link

sizebot commented Mar 28, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against a86b002

We store an effect pointer so we can backtrack in the effect list in some
cases. This is a stateful variable. If we interrupt a render we need to
reset it.

This field was added after the optimization was added and I didn't remember
to reset it here.

Otherwise we end up not resetting the firstEffect so it points to a stale
list. As a result children don't end up inserted like we think they were.
Then we try to remove them it errors.

It would be nicer to just get rid of the effect list and use the tree for
effects instead. Maybe we still need something for deletions tho.
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand it but I trust you this works

() => updateHighPri(true),
);

// That will intercept the previous render.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interrupt? :-)

// First attempt at high pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh that makes sense! I didn’t realize it works this way.

// Re-render at forced.
'Suspend! [A]',
'Loading A',
// We auto-commit this on DEV.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? "Flush fallbacks" flag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea

'Suspend! [A]',
'Loading A',
]
: [
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants