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.

Sign up for GitHub

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): Add FAST_REFRESH config flag #28409

Merged
merged 4 commits into from
Dec 1, 2020
Merged

fix(gatsby): Add FAST_REFRESH config flag #28409

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
55bf157
add FAST_REFRESH flag
LekoArts Dec 1, 2020
9c0cd97
update desc
LekoArts Dec 1, 2020
c803d2b
handle case when FAST_REFRESH flag is set and GATSBY_HOT_LOADER env v…
pieh Dec 1, 2020
b05e2bf
Merge branch 'master' into fast-refresh/config-flag
pieh Dec 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/gatsby/cache-dir/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ import syncRequires from "$virtual/sync-requires"
// Generated during bootstrap
import matchPaths from "$virtual/match-paths.json"

if (process.env.GATSBY_HOT_LOADER === `fast-refresh` && module.hot) {
module.hot.accept(`$virtual/sync-requires`, () => {
// Manually reload
})
}

window.___emitter = emitter

const loader = new DevLoader(syncRequires, matchPaths)
Expand Down
21 changes: 19 additions & 2 deletions packages/gatsby/src/services/initialize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,6 @@ export async function initialize({
reporter.panic(`Missing program args`)
}

process.env.GATSBY_HOT_LOADER = getReactHotLoaderStrategy()

/* Time for a little story...
* When running `gatsby develop`, the globally installed gatsby-cli starts
* and sets up a Redux store (which is where logs are now stored). When gatsby
Expand Down Expand Up @@ -184,6 +182,23 @@ export async function initialize({

// Setup flags
if (config && config.flags) {
// TODO: this should be handled in FAST_REFRESH configuration and not be one-off here.
if (
config.flags.FAST_REFRESH &&
process.env.GATSBY_HOT_LOADER &&
process.env.GATSBY_HOT_LOADER !== `fast-refresh`
) {
delete config.flags.FAST_REFRESH
reporter.warn(
reporter.stripIndent(`
Both FAST_REFRESH gatsby-config flag and GATSBY_HOT_LOADER environment variable is used with conflicting setting ("${process.env.GATSBY_HOT_LOADER}").

Will use react-hot-loader.

To use Fast Refresh either do not use GATSBY_HOT_LOADER environment variable or set it to "fast-refresh".
`)
)
}
const availableFlags = require(`../utils/flags`).default
// Get flags
const { enabledConfigFlags, unknownFlagMessage, message } = handleFlags(
Expand Down Expand Up @@ -216,6 +231,8 @@ export async function initialize({
}
}

process.env.GATSBY_HOT_LOADER = getReactHotLoaderStrategy()

// theme gatsby configs can be functions or objects
if (config && config.__experimentalThemes) {
reporter.warn(
Expand Down
9 changes: 9 additions & 0 deletions packages/gatsby/src/utils/flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ const activeFlags: Array<IFlag> = [
description: `Don't process images during development until they're requested from the browser. Speeds starting the develop server.`,
umbrellaIssue: `https://github.com/gatsbyjs/gatsby/discussions/27603`,
},
{
name: `FAST_REFRESH`,
env: `GATSBY_FAST_REFRESH`,
command: `develop`,
telemetryId: `FastRefresh`,
experimental: false,
description: `Use React Fast Refresh instead of the legacy react-hot-loader for instantaneous feedback in your development server. Recommended for versions of React >= 17.0.`,
umbrellaIssue: `https://github.com/gatsbyjs/gatsby/discussions/28390`,
},
]

export default activeFlags
5 changes: 4 additions & 1 deletion packages/gatsby/src/utils/get-react-hot-loader-strategy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@ export function getReactHotLoaderStrategy(): string {
// If the user has defined this, we don't want to do any package sniffing
if (process.env.GATSBY_HOT_LOADER) return process.env.GATSBY_HOT_LOADER

// If the config flag is true, return fast-refresh
if (process.env.GATSBY_FAST_REFRESH) return `fast-refresh`
Comment on lines 8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

This order of conditions has confusing impact on terminal output when user adds FAST_REFRESH to config flags, but still use GATSBY_HOT_LOADER=react-hot-loader:

➜  gatsby-starter-blog git:(master) ✗ GATSBY_HOT_LOADER=react-hot-lodear gatsby develop
info The following flags are active:
- FAST_REFRESH · (Umbrella Issue (​https://github.com/gatsbyjs/gatsby/discussions/28390​)) · Use React Fast Refresh instead of the legacy react-hot-loader for instantaneous feedback in your development
 server. Recommended for versions of React >= 17.0.

but in the end react-hot-loader will be used because this code as-is prefers it

It's tricky because it feels like env vars should have priority over config (same as cli toggles over any config in file). So the order now seems correct - just the message is confusing :/

Copy link
Contributor

@pieh pieh Dec 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

maybe before we handle flags we need to do some hacky stuff like change

gatsby/packages/gatsby/src/services/initialize.ts

Lines 186 to 187 in 7577e3f

if (config && config.flags) {
const availableFlags = require(`../utils/flags`).default

to be something like

  if (config && config.flags) {
    if(config.flags.?FAST_REFRESH && process.env.GATSBY_HOT_LOADER) {
      delete config.flags.FAST_REFRESH
      reporter.warn(`Both FAST_REFRESH config flag and GATSBY_HOT_LOADER env var is used. Will use env var`)
    }
    const availableFlags = require(`../utils/flags`).default

Copy link
Contributor

Choose a reason for hiding this comment

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

I like hacky! Well temporarily hacky that we have nice TODOs about fixing but that unblocks us to ship.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed c803d2b -
Screenshot 2020-12-01 at 18 06 43


// Do some package sniffing to see if we can use fast-refresh if the user didn't
// specify a specific hot loader with the environment variable.

// TODO: Decide if we wanna do this
// TODO: Probably use the flags for this
/*
try {
const reactVersion = require(`react/package.json`).version
Expand Down