-
Notifications
You must be signed in to change notification settings - Fork 674
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(gatsby-plugin-theme-ui): Fast Refresh Compatibility #1659
fix(gatsby-plugin-theme-ui): Fast Refresh Compatibility #1659
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/systemui/theme-ui/33zAQ1LqsCE6mhnXqrVMxJoaVSuv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great! Super appreciate your getting to the bottom of this.
@hasparus Any idea why we're having this build error? Idk why this unchanged file would be erroring now if it's not erroring currently |
Thanks so much @LekoArts! I'm pushing a small fix to the docs to your branch if you don't mind.
@lachlanjc We didn't spread pageContext onto Layout anymore. (Insert me blabbering about TypeScript for 5 minutes again) I'm good to merge it whenever you want. |
Good monday folks! I saw the failure at the end of my day but thanks for fixing it already! From my end everything would be ready to go. In case you were wondering where you should release this: Once we merge & release gatsbyjs/gatsby#30901 for Theme UI it should work even when people not upgrade, so feel free to release it in whatever version you prefer. |
If we're all ready to do it, let's do it! Thanks again. |
🚀 PR was released in |
Thanks! |
Description
Hi!
We were looking into the issues listed below and found that we'd need to fix a little thing in Gatsby and also something in Theme UI. Fast Refresh has some limitations (https://www.gatsbyjs.com/docs/reference/local-development/fast-refresh/#limitations) and in the past our docs didn't account for those + the Theme UI plugin is pretty "old" so it used the suggested solutions at the time.
Anyhow, the issue is that the
wrapRootElement
/wrapPageElement
APIs in Gatsby are not React components but functions that we call and so in this plugin they pass the lowercased component through. Fast Refresh doesn't like lowercased components so the fix is to 1) use PascalCase for the real react component and 2) use that React component explicitly ingatsby-ssr.js
/gatsby-browser.js
.While our fix on Gatsby's side of things will make Theme UI work, too, adding these changes here will minimize the React tree that Fast Refresh has to go through. Here's a video:
2021_04_16_3fed8cd9.mp4
As you can see only two files were traversed for the change. And it works, of course 😆
This PR
I've put each change in separate commits for easier reviewing. I essentially applied the changes made to the Gatsby plugin to all other Gatsby instances where that pattern was used.
I also updated the docs and any occurrence of a theme/components to not use anonymous default exports (as Fast Refresh doesn't like that) and it's also better for debugging / profiling.
Related
Fixes #1440
Fixes gatsbyjs/gatsby#30387
Fixes #1595
Version
Published prerelease version:
v0.7.1-develop.0
Changelog
🎉 This release contains work from a new contributor! 🎉
Thank you, Jonathan Van Buren (@vanbujm), for all your work!
🐛 Bug Fix
gatsby-plugin-theme-ui
@theme-ui/components
develop
@theme-ui/editor
,gatsby-plugin-theme-ui
,@theme-ui/parse-props
,@theme-ui/prism
,@theme-ui/style-guide
@theme-ui/style-guide
🔩 Dependency Updates
Authors: 5