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

Fix suspense fallback throttling #24253

Merged
merged 4 commits into from
Apr 12, 2022

Conversation

sunderls
Copy link
Contributor

@sunderls sunderls commented Apr 1, 2022

Summary

There seems to be a regression on suspense throttling.

in React 18-alpha version, Suspense fallbacks are throttled in committing codesandbox link
in React 18 release version, this is no longer working codesandbox link

Root cause

see the inline comments

How did you test this change?

before after
Apr-02-2022.00-24-42.mp4
Apr-02-2022.00-25-25.mp4

Test

Added one more test case in ReactSuspense-test.internal.js, to mimic the repro in above videos

on main branch it fails

Screenshot 2022-04-04 at 23 58 32

in this patch, it passes

Screenshot 2022-04-04 at 23 59 21

@sizebot
Copy link

sizebot commented Apr 1, 2022

Comparing: e0160d5...8bd48ca

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.03% 131.28 kB 131.32 kB +0.04% 41.94 kB 41.96 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.03% 136.55 kB 136.58 kB +0.03% 43.53 kB 43.55 kB
facebook-www/ReactDOM-prod.classic.js +0.03% 435.21 kB 435.34 kB +0.02% 79.86 kB 79.87 kB
facebook-www/ReactDOM-prod.modern.js +0.03% 420.20 kB 420.34 kB +0.02% 77.49 kB 77.51 kB
facebook-www/ReactDOMForked-prod.classic.js +0.03% 435.21 kB 435.34 kB +0.02% 79.86 kB 79.87 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 8bd48ca

@sunderls sunderls force-pushed the fix-suspense-throttling branch from fd70a75 to d9a9e7c Compare April 1, 2022 23:38
@sunderls sunderls force-pushed the fix-suspense-throttling branch 2 times, most recently from d45bb57 to 9805935 Compare April 1, 2022 23:57
@gaearon
Copy link
Collaborator

gaearon commented Apr 2, 2022

It would be nice to add some test that passes in this branch but fails on main.

@acdlite
Copy link
Collaborator

acdlite commented Apr 4, 2022

Nice fix! I can work on adding a test unless you beat me to it

@sunderls sunderls force-pushed the fix-suspense-throttling branch from 9805935 to a2bfae1 Compare April 4, 2022 22:59
@sunderls sunderls force-pushed the fix-suspense-throttling branch from a2bfae1 to b1062aa Compare April 4, 2022 23:11
@sunderls sunderls changed the title fix suspense throttling fix suspense fallback throttling Apr 4, 2022
@sunderls sunderls marked this pull request as ready for review April 4, 2022 23:24
@sunderls
Copy link
Contributor Author

sunderls commented Apr 4, 2022

@gaearon @acdlite I've added a test case which fails on main branch.
Hope you can take a look

@sunderls sunderls requested a review from gaearon April 9, 2022 12:31
@sunderls sunderls force-pushed the fix-suspense-throttling branch from 345b90f to d89c871 Compare April 9, 2022 20:01
@gaearon gaearon changed the title fix suspense fallback throttling Fix suspense fallback throttling Apr 11, 2022
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.

Looks good to me. Leaving for @acdlite to do a final review & merge

@gaearon
Copy link
Collaborator

gaearon commented Apr 11, 2022

@sunderls Thank you so much for finding this and sending a fix! Really nice work.

@acdlite
Copy link
Collaborator

acdlite commented Apr 12, 2022

Very impressive PR! Thanks for the fix!

@acdlite acdlite merged commit 4fc394b into facebook:main Apr 12, 2022
@sunderls sunderls deleted the fix-suspense-throttling branch April 12, 2022 18:06
rickhanlonii pushed a commit that referenced this pull request Apr 13, 2022
* fix suspense throttling

* fix lint

* Tweak tests + another test

Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
rickhanlonii pushed a commit that referenced this pull request Apr 14, 2022
* fix suspense throttling

* fix lint

* Tweak tests + another test

Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* fix suspense throttling

* fix lint

* Tweak tests + another test

Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
rickhanlonii pushed a commit that referenced this pull request Apr 19, 2022
* fix suspense throttling

* fix lint

* Tweak tests + another test

Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
This was referenced Nov 8, 2024
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.

5 participants