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 unnecessary top-level context changes #1924

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Feb 1, 2022

MDXProvider often sits at the top of the trees. The object spread here is unfortunate because it means the provider value will always be new, and so React would need to spend time propagating the context (even though nothing really changed). This also has implications on Selective Hydration in React 18: if top-level context updates before hydration is fully finished, we have to show Suspense fallbacks — which isn't a great experience when it's unnecessary. facebook/react#22692.

This change should fix it.

How to test

I typed this change on GH so you'll need to double-check but hopefully this looks good.

Locally, I tested something similar with manual editing in node_modules, and it worked.

My actual "test case" is in reactjs/react.dev#4256. When I do this fix together with vercel/next.js#33861, HTML from code-split chunks is no longer forced to turn into a fallback. I've artificially increased the bundle load delay in this video to make the hydration noticeable. When code blocks become colored, it's hydrated:

yo.mov

Before the change, the code blocks would turn into placeholders before turning back into codeblocks.

@vercel
Copy link

vercel bot commented Feb 1, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mdx/mdx/3tvkGxFofbz2F6JK9KGgsMMkNhEi
✅ Preview: https://mdx-git-fork-gaearon-patch-1-mdx.vercel.app

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2022

not sure how to fix the ts thing, do you want to do it or i'll look into it?

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2022

would it be tough to also port this to 1.x? not sure if you release those anymore

@wooorm
Copy link
Member

wooorm commented Feb 1, 2022

  • I was planning on released v2 today, so, right on time for that!
  • repo for v1 is severely broken, I don’t think I dare touch it, especially for features/improvements, rather than security vulnerabilities. Can you get around it by using ESM for the provider / patch-package?

@gaearon
Copy link
Contributor Author

gaearon commented Feb 1, 2022

fair enough, maybe i'll just bite the bullet and try to get the v2 migration going again

@wooorm wooorm merged commit 9ca9d40 into mdx-js:main Feb 1, 2022
@wooorm
Copy link
Member

wooorm commented Feb 1, 2022

Thanks дэн!

@wooorm wooorm added 🏁 area/perf This affects performance 🐛 type/bug This is a problem 👶 semver/patch This is a backwards-compatible fix 💪 phase/solved Post is done 🗄 area/interface This affects the public interface labels Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🏁 area/perf This affects performance 💪 phase/solved Post is done 👶 semver/patch This is a backwards-compatible fix 🐛 type/bug This is a problem
Development

Successfully merging this pull request may close these issues.

2 participants