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): Support FastRefresh for development hot reloading #21534

Merged
merged 14 commits into from
Mar 11, 2020

Conversation

blainekasten
Copy link
Contributor

@blainekasten blainekasten commented Feb 17, 2020

Description

React development is under iteration with the release of the official React FastRefresh hot reloading approach. Platform integration is under way and this allows Gatsby to be configured with fast refresh as the mechanism for hot reloading.

This feature is currently hidden behind an environment variable GATSBY_HOT_LOADER=fast-refresh. The reason we have to do this is because we support a wide semver range of React (16.4+). FastRefresh requires 16.10+. So if we were to change this as the default, it would be a breaking feature for those on previous versions.

My proposal is to ship this, and when we update the semver range we can remove the old hot-loader mechanism.

I tested this approach with multiple versions and flags:

  • 16.4.8 (no flag) works with hot-loader
  • 16.4.8 (flag on) does not do any hot updates (as expected)
  • 16.12 (no flag) works with hot-loader
  • 16.12 (flag on) works with fast refresh

Documentation

I don't think this will need any documentation changes unless we have some documentation specifically calling out react-hot-loader. Principally, this is an implementation detail change.

Related Issues

@blainekasten blainekasten requested a review from a team as a code owner February 17, 2020 19:47
@KyleAMathews
Copy link
Contributor

Can we detect the react version in our webpack config and adjust there?

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Oh this looks awesome! 🎉 I wonder if it would be beneficial to move js process.env.HOT_LOADER !== `fast-refresh` into a utils functions called isFastRefreshEnabled or something.

Can we detect the react version in our webpack config and adjust there?

@KyleAMathews I think we want to run it as an experiment first as it's still considered experimental in the webpack ecosystem.

packages/gatsby/src/utils/webpack.config.js Outdated Show resolved Hide resolved
@pieh
Copy link
Contributor

pieh commented Feb 18, 2020

Also this is not only about whether user have 16.10+ react version (so if it will work at all) - react-refresh have different behaviour than react-hot-loader - particularly it will always blow away state for Class components on refreshes - so changing it without explicit opt in will be BREAKING CHANGE for some codebases. So we just can't make it default in v2 (unless we want breaking changes without major version bump)

@blainekasten
Copy link
Contributor Author

blainekasten commented Feb 18, 2020

@pieh I think BREAKING CHANGE might be a misnomer. This doesn't affect users codebases, or require them to do anything different. It might change their workflow but does not break their development or build.

It is a really good point though. So good call out. I was going to change this to use fast refresh if react is 16.10+. But now i'm questioning that decision.

@blainekasten
Copy link
Contributor Author

@KyleAMathews I think we want to run it as an experiment first as it's still considered experimental in the webpack ecosystem.

@wardpeet it's not considered experimental, it just doesn't have a champion owning it's adoption. It's already built into react native, and facebook uses it internally with haste. And the react-hot-loader repo has this line:

React-Hot-Loader is expected to be replaced by React Fast Refresh. Please remove React-Hot-Loader if Fast Refresh is currently supported on your environment.

@pieh
Copy link
Contributor

pieh commented Feb 18, 2020

Agree about misnomer - it's not technically breaking change. But still if my workflow would "rely" on current behaviour (iterate on code relying on current hot reload behaviour) and we change that behaviour for every user - my workflow is now broken. Was the user code structured using best practices for it to happen - no, but it did work.

There are very good reason that react-refresh doesn't try to keep class component state, but some people can rely on it. Changing defaults like that is what cause some people to not update gatsby because they never know what can change/break. IMO: We should ship it, but behind feature flag (like it is right now). IMO: Changing defaults that result in any behaviour changes should be reserved for major versions.

@wardpeet
Copy link
Contributor

wardpeet commented Feb 18, 2020

@blainekasten It's still experimental for react web. All loaders (webpack & parcel) are still considered experimental is what I meant.

That's also why I think process.env is the right way to go for now.

I also agree with Michal

@blainekasten
Copy link
Contributor Author

@pieh / @wardpeet what do you think about adding a config to gatsby-config so it's easier to surface and see? Is there value in that?

@pieh
Copy link
Contributor

pieh commented Feb 18, 2020

I don't want to commit to conversation about ways to improve visibility about feature flags just yet - mostly because this (not enabling by default) is my personal view and it doesn't necessarily reflect view of all the people here ;)

@wardpeet
Copy link
Contributor

@pieh / @wardpeet what do you think about adding a config to gatsby-config so it's easier to surface and see? Is there value in that?

I don't have an issue with that but I know we weren't big fans of that. Honestly, I don't know why.

@blainekasten
Copy link
Contributor Author

Oh this looks awesome! 🎉 I wonder if it would be beneficial to move js process.env.HOT_LOADER !== fast-refresh into a utils functions called isFastRefreshEnabled or something.

@wardpeet I looked into this. Unfortunately, part of the changes are from cache-dir, which does not seem to have a safe and obvious way to import from gatsby internals (tell me if i'm wrong). It looks like we would have to expose some sort of flags on the gatsby package or something. Which feels like a larger conversation.

So for now we'll just leave the env var littered throughout the codebase..

@pieh pieh added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Feb 21, 2020
@blainekasten
Copy link
Contributor Author

https://github.com/pmmmwh/react-refresh-webpack-plugin/releases/tag/v0.2.0 is released! This is unblocked and should be mergeable

@blainekasten blainekasten changed the title feat(gatsby) Support FastRefresh for development hot reloading feat(gatsby): Support FastRefresh for development hot reloading Mar 3, 2020
@blainekasten blainekasten added this to the Next Major milestone Mar 3, 2020
@blainekasten blainekasten added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Mar 4, 2020
@blainekasten blainekasten removed the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Mar 6, 2020
madalynrose
madalynrose previously approved these changes Mar 6, 2020
Copy link
Contributor

@madalynrose madalynrose 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 merge after adding in some comments about where things live!

@blainekasten blainekasten added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Mar 11, 2020
@blainekasten blainekasten merged commit 205847b into master Mar 11, 2020
@delete-merged-branch delete-merged-branch bot deleted the experimental/fast-refresh branch March 11, 2020 15:54
@adrienharnay
Copy link
Contributor

Hey, I am using gatsby in a monorepo (yarn workspaces), and am seeing this error since the update:

 ERROR #98123  WEBPACK

Generating development JavaScript bundle failed

Can't resolve '@pmmmwh/react-refresh-webpack-plugin/src/overlay' in '/fakepath/packages/service/.cache'

File: .cache/error-overlay-handler.js

Any idea why?

@pieh
Copy link
Contributor

pieh commented Mar 31, 2020

@adrienharnay I would try force re-installing dependencies (yarn --force), if that doesn't fix it, we might ask for access to a project to debug the problem.

@adrienharnay
Copy link
Contributor

Tried yarn --force but no dice. The only thing that "fixed" it was to specifically install @pmmmwh/react-refresh-webpack-plugin, because else it is in node_modules/gatsby/node_modules/ and cannot be accessed... I wonder if there's anything to be done about it

@pieh
Copy link
Contributor

pieh commented Mar 31, 2020

Ah yeah, the react-refresh webpack package is not being hoisted (being in root node_modules and that cause gatsby to fail to find it. Definitely something we should fix. TBD yet on how. Could you create issue for it? Comments in merged PRs are easy to forget ;(

@adrienharnay
Copy link
Contributor

Of course, thanks for the quick answer!

@sylvesteraswin
Copy link

@blainekasten is there a way to know if fast-refresh is enabled?

@blainekasten
Copy link
Contributor Author

@sylvesteraswin right now you can check if the environment variable is set. In the future when we don't have that it'll just be the default. Why do you need to know?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes breaking change If implemented, this proposed work would break functionality for older versions of Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants