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

Merged
merged 4 commits into from
Dec 1, 2020
Merged

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Dec 1, 2020

Description

Adds a FAST_REFRESH config flag so that users can more easily enable it. Previously they had to set REACT_HOT_LOADER=fast-refresh (this still works).

This PR also adds a small improvement to app.js for reloading.

[ch19923]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 1, 2020
@LekoArts LekoArts added topic: hot reloading* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 1, 2020
KyleAMathews
KyleAMathews previously approved these changes Dec 1, 2020
Comment on lines 8 to +11
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`
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

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

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

…ar is set to something else than fast-refresh
pieh
pieh previously approved these changes Dec 1, 2020
@pieh pieh merged commit ce090e5 into master Dec 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the fast-refresh/config-flag branch December 1, 2020 18:36
pieh pushed a commit that referenced this pull request Dec 1, 2020
* add FAST_REFRESH flag

* update desc

* handle case when FAST_REFRESH flag is set and GATSBY_HOT_LOADER env var is set to something else than fast-refresh

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit ce090e5)
pieh pushed a commit that referenced this pull request Dec 1, 2020
* add FAST_REFRESH flag

* update desc

* handle case when FAST_REFRESH flag is set and GATSBY_HOT_LOADER env var is set to something else than fast-refresh

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit ce090e5)
pieh pushed a commit that referenced this pull request Dec 1, 2020
* add FAST_REFRESH flag

* update desc

* handle case when FAST_REFRESH flag is set and GATSBY_HOT_LOADER env var is set to something else than fast-refresh

Co-authored-by: Michal Piechowiak <[email protected]>
(cherry picked from commit ce090e5)

Co-authored-by: Lennart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants