Skip to content

[SuspenseList] Don't treat the last row in hidden as deleted if already mounted #17206

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

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

sebmarkbage
Copy link
Collaborator

Already mounted rows that resuspend may be considered as part of a tail if they're at the end. However, for purposes of the tail="..." option they don't get deleted. We deal with that in cutOffTailIfNeeded.

However, if they're also the first to suspend in the "hidden" case, we have a special case that deletes the actual rendered row. This needs to consider if that row was already mounted or things go wrong.

@sebmarkbage sebmarkbage changed the title Don't treat the last row in hidden as deleted if already mounted [SuspenseList] Don't treat the last row in hidden as deleted if already mounted Oct 29, 2019
@sebmarkbage
Copy link
Collaborator Author

For the purposes of using the "tail" option you probably don't really ever care about already mounted components being part of the tail and we could just consider all of them as part of the head.

However, when you don't use it an update causes a resuspend it might be a bit confusing that they're now in "together" mode (since the head is always in "together" mode).

It would simplify things a bit if we just dropped this for updates though.

@sizebot
Copy link

sizebot commented Oct 29, 2019

Size changes (stable)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 2ff0f18

@sizebot
Copy link

sizebot commented Oct 29, 2019

Size changes (experimental)

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 2ff0f18

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I'm not sure I get all of the nuance in this PR to be honest. Seems ok?

@@ -2135,4 +2135,159 @@ describe('ReactSuspenseList', () => {
</div>,
);
});

it('eventually resolves two nested forwards suspense list with a hidden tail', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "forwards suspense lists"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oops. this is a copy paste of the test above.

Already mounted rows that resuspend may be considered as part of a tail
if they're at the end. However, for purposes of the tail="..." option
they don't get deleted. We deal with that in cutOffTailIfNeeded.

However, if they're also the first to suspend in the "hidden" case, we have
a special case that deletes the actual rendered row. This needs to consider
if that row was already mounted or things go wrong.
@sebmarkbage sebmarkbage merged commit 6cd365c into facebook:master Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants