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

feat(gatsby): Add preliminary fast-refresh integration #26664

Merged
merged 54 commits into from
Nov 26, 2020

Conversation

blainekasten
Copy link
Contributor

@blainekasten blainekasten commented Aug 27, 2020

Description

TL;DR: Custom overlay, removing page query export, adding some tests


Fast Refresh is supported with React 16.8+. Previously we've only been using FastRefresh if users specifically set GATSBY_HOT_LOADER=fast-refresh. The problem here is that React 17 does not work with react-hot-loader and so the better option here is to detect if the react version supports fast refresh.

This PR is to sniff into react's version and detect if fast refresh is usable. The user is still able to override it with GATSBY_HOT_LOADER=react-hot-loader.
We'll revisit this in another PR.

This PR also ensures that page queries are removed via babel because with FastRefresh, updates scan upwards through their module tree to scan for usages to know what modules all need updating. With page queries there is a big problem because they are never imported, the end effect is then that the FastRefresh can't know if it's safe to update and will reject the update requiring the user to reload. This is bad because page queries are quite common.

This is potentially problematic with #20672, so we might need to address that before this can be merged. nvm

This PR/implementation is not perfect and most likely will be refactored when we release it for everyone, until then this is good enough. Some TODOs were left in the code on purpose, not needed to be addressed in this PR.

Documentation

Only when Fast Refresh is available for everyone (as we e.g. bump the minimum React version) we need to change docs.

Related Issues

[ch19063]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 27, 2020
@gatsby-cloud

This comment has been minimized.

@gatsby-cloud

This comment has been minimized.

@blainekasten blainekasten added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Aug 27, 2020
@gatsby-cloud
Copy link

gatsby-cloud bot commented Aug 27, 2020

Gatsby Cloud Build Report

gatsby

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 5m

Copy link
Contributor

@smthomas smthomas left a comment

Choose a reason for hiding this comment

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

Just one question, otherwise LGTM.

@gatsby-cloud

This comment has been minimized.

@blainekasten
Copy link
Contributor Author

Not sure how to move forward with this now. The tests won't pass while multiple versions of React are installed (e.g., installing React 16.9+ for the e2e tests to use fast refresh while gatsby-cli installs 16.8). We need to find a way to decouple the react version from gatsby-cli. I'm afraid we're only going to run into more and more of these problems as we work to support React 17.

is this now proving to be a requirement for Gatsby v3?

@pieh
Copy link
Contributor

pieh commented Sep 14, 2020

I just opened #26887 that removes react from gatsby-cli dependencies (moves to devDeps and bundle it) which should let us move past those problems.

@blainekasten
Copy link
Contributor Author

@pieh awesome! Ping me when it goes through and I'll update this branch and hopefully the tests pass then!

@LekoArts LekoArts force-pushed the fast-refresh/use-if-react-supports branch from 69d5922 to f01cccc Compare November 10, 2020 09:18
@blainekasten blainekasten requested a review from a team as a code owner November 10, 2020 09:18
@LekoArts LekoArts self-assigned this Nov 11, 2020
@LekoArts LekoArts force-pushed the fast-refresh/use-if-react-supports branch from 857459b to fd25b48 Compare November 11, 2020 14:10
@LekoArts LekoArts changed the title chore(gatsby): Use fast refresh if the version of react installed supports it chore(gatsby): Add preliminary fast-refresh integration Nov 26, 2020
@LekoArts LekoArts changed the title chore(gatsby): Add preliminary fast-refresh integration feat(gatsby): Add preliminary fast-refresh integration Nov 26, 2020
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Yes! 🎉

We still have to make decision on enabling by default for react@17, but we shouldn't block all the improvements done here on that.

@pieh pieh merged commit 613f5c7 into master Nov 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the fast-refresh/use-if-react-supports branch November 26, 2020 18:11
@LekoArts LekoArts added the topic: DX Developer Experience (e.g. Fast Refresh, i18n, SSR, page creation, starters) label May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) topic: DX Developer Experience (e.g. Fast Refresh, i18n, SSR, page creation, starters)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants