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(gatsby-plugin-theme-ui): Fast Refresh Compatibility #1659

Merged
merged 5 commits into from
Apr 19, 2021
Merged

fix(gatsby-plugin-theme-ui): Fast Refresh Compatibility #1659

merged 5 commits into from
Apr 19, 2021

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Apr 16, 2021

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 in gatsby-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
    • fix(components): Allow styled-system space props on Paragraph #1658 (@vanbujm)

⚠️ Pushed to develop

  • ci(auto): temporarily disable auto/magic-zero (@hasparus)
  • chore: gitignore yarn-error.log (@hasparus)
  • chore: add auto magic-zero plugin (@hasparus)
  • @theme-ui/editor, gatsby-plugin-theme-ui, @theme-ui/parse-props, @theme-ui/prism, @theme-ui/style-guide
    • chore: update internal peerDependencies (@hasparus)
  • @theme-ui/style-guide
    • chore(style-guide): widen theme-ui peerDependency (@hasparus)

🔩 Dependency Updates

Authors: 5

@vercel
Copy link

vercel bot commented Apr 16, 2021

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/systemui/theme-ui/33zAQ1LqsCE6mhnXqrVMxJoaVSuv
✅ Preview: https://theme-ui-git-fork-lekoarts-fix-gatsby-fast-refresh-systemui.vercel.app

@LekoArts LekoArts changed the title examples fix(gatsby-plugin-theme-ui): Fast Refresh Compatibility Apr 16, 2021
Copy link
Member

@lachlanjc lachlanjc left a 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.

@lachlanjc
Copy link
Member

@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
image

@hasparus
Copy link
Member

Thanks so much @LekoArts! I'm pushing a small fix to the docs to your branch if you don't mind.

Idk why this unchanged file would be erroring now if it's not erroring currently

@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.

@LekoArts
Copy link
Contributor Author

LekoArts commented Apr 19, 2021

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.

@lachlanjc lachlanjc merged commit ce95a2d into system-ui:develop Apr 19, 2021
@lachlanjc
Copy link
Member

If we're all ready to do it, let's do it! Thanks again.

@hasparus
Copy link
Member

🚀 PR was released in v0.7.1-develop.0 🚀

@hasparus hasparus added the prerelease This change is available in a prerelease. label Apr 19, 2021
@LekoArts LekoArts deleted the fix-gatsby-fast-refresh branch April 19, 2021 14:37
@LekoArts
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prerelease This change is available in a prerelease.
Projects
None yet
3 participants