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

Add a failing test for remounting fallback in sync mode #14078

Closed
wants to merge 1 commit into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 2, 2018

This is a failing test for #14073.
See last line for the failing case.

I'm not actually sure it's a bug. It makes sense that if we render null in sync mode, we end up remounting. But it seems like far from ideal and I wonder if we can fix it.

'Child 2',
'Suspend! [Child 3]',
'Loading...',
'Suspend! [Child 3]', // TODO: why does this suspend twice?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was also weird

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a known flaw with sync mode. Pings aren't batched, so we get a synchronous retry for every individual ping. Added a comment in the rebased version of your test:

// TODO: This suspends twice because there were two pings, once for each
// time Child 2 suspended. This is only an issue in sync mode because
// pings aren't batched.

@sizebot
Copy link

sizebot commented Nov 2, 2018

Details of bundled changes.

Comparing: c84b9bf...de878eb

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@acdlite
Copy link
Collaborator

acdlite commented Nov 2, 2018

I was thinking that one solution to this would be to render the fallback in the first commit, and hide the timed out tree in the second commit. It’s annoying though because it adds so much complexity just for a legacy case. I think we should wait to see if this is a problem in practice first.

@gaearon
Copy link
Collaborator Author

gaearon commented Nov 2, 2018

It does seem to be a problem in practice as described in #14073 — sucks for the spinner to "twitch".

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