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): Set up Fast Refresh #29588

Merged
merged 23 commits into from
Feb 26, 2021
Merged

feat(gatsby): Set up Fast Refresh #29588

merged 23 commits into from
Feb 26, 2021

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Feb 19, 2021

Description

This PR improves our current Fast Refresh integration. Instead of completely disabling the overlay for the ReactRefreshWebpackPlugin we now pass a custom module and rely on the whm integration. We also needed to vendor webpack-hot-middleware.

List of changes made:

  • GraphQL errors (handled in error-overlay-handler) are also pushed to our new overlay now
  • Instead of directly relying on mitt we push to a queue in window._gatsbyEvents because sometimes we need actions before React rehydrated and mitt was too late for that then
  • Rewrite the frontend code (inside cache-dir/fast-refresh-overlay) to use named exports, more modular components
  • Adapted components and code to account for new Webpack 5 shape + changed the sourcing for source-maps. compilation.codeGenerationResults inside start-server.ts
  • Use an Accordion to display multiple runtime/graphql errors
  • Add helpers to lock the body and trap focus
  • Rewrite the fast-refresh-overlay/index.js to use a reducer for its state logic
  • Use a custom fast-refresh-module for the module option and expose all expected options

Documentation

In case we have pages where we show an error overlay screenshot, those would need to be updated.

Testing

In case you want to test it, here's a src/pages/index.js file I used for development.

import React from "react"
import { graphql } from 'gatsby'

// Compile error: Remove the } from ({ data })
const IndexPage = ({ data }) => {
  // Multiple runtime errors
  // React.useEffect(() => {
  //   setTimeout(() => {
  //     window.blabla()
  //   }, 100)

  //   setTimeout(() => {
  //     window.blabla2()
  //   }, 150)
  // })

  // Runtime error
  // const foo = null;foo.bar = 'bar'

  return (
    <main>
      <div>
        Hello
      </div>
      <pre>{JSON.stringify(data, null, 2)}</pre>
    </main>
  )
}

export default IndexPage

/*
Possible changes to GraphQL query:
siteMetadata {
  title2
  test
}

Or

site {
  buildTime(format: "ASDF")
}
*/

export const query = graphql`
  {
    site {
      buildTime
      siteMetadata {
        title
      }
    }
  }
`

Related Issues

Initial integration: #26664

[ch25595]
[ch25647]
[ch19583]
[ch25648]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 19, 2021
@LekoArts LekoArts removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 19, 2021
@LekoArts LekoArts added the topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) label Feb 24, 2021
@LekoArts LekoArts marked this pull request as ready for review February 25, 2021 08:50
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.

I didn't go through the whole code but this is what I found with a rough glimpse

Comment on lines +21 to +34
React.useEffect(() => {
async function fetchData() {
const res = await fetch(url)
const json = await res.json()
const decoded = prettifyStack(json.codeFrame)
const { sourcePosition, sourceContent } = json
setResponse({
decoded,
sourceContent,
sourcePosition,
})
}
fetchData()
}, [])
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably want to use abortcontroller here so the fetch aborts when we unmount.

A dirty fix could be to add a is Mounted ref and not set the response when it's false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it with this and it completely broke the current working behavior:

  const controller = new AbortController()
  const { signal } = controller

  React.useEffect(() => {
    async function fetchData() {
      const res = await fetch(url, { signal })
      const json = await res.json()
      const decoded = prettifyStack(json.codeFrame)
      const { sourcePosition, sourceContent } = json
      setResponse({
        decoded,
        sourceContent,
        sourcePosition,
      })
    }
    fetchData()

    return () => {
      controller.abort()
    }
  }, [])

Copy link
Contributor

Choose a reason for hiding this comment

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

:o you need to ue refs but let's do it after :)

return null
})
.filter(Boolean)
errorsToDisplay = errors.flatMap(e => e).filter(Boolean)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary to do this handling here, I grab all info in the graphql-error component itself.
Also, we can use flatMap because we're now at Node 12.13 👍 and that's a Node 11 feature.

}) {
const [isOpen, setIsOpen] = React.useState(open)
const [prevIsOpen, setPrevIsOpen] = React.useState(open)
const id = useId(`accordion-item`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need a stable id here (for both SSR and client for re-hydration) so that's why this helper exists

}

render() {
return this.state.hasError ? null : this.props.children
// Without this check => possible infinite loop
return this.state.error && this.props.hasErrors ? null : this.props.children
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Necessary to also have this.state.error so that the background doesn't vanish

<Backdrop />
<div
data-gatsby-overlay="root"
role="alertdialog"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purposefully chose alertdialog here and not dialog.

The difference with regular dialogs is that the alertdialog role should only used when an alert, error, or warning occurs. In other words, when a dialog's information and controls require the user's immediate attention alertdialog should be used instead of dialog. It is up to the developer to make this distinction though.

This fits our use case here

packages/gatsby/package.json Outdated Show resolved Hide resolved
<h1 id="gatsby-overlay-labelledby">Failed to compile</h1>
<span>{file}</span>
</div>
<HeaderOpenClose open={() => openInEditor(file, 1)} dismiss={false} />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a follow up: we should put this openInEditor on the same place as runtime errors so people know where to look.

@wardpeet wardpeet changed the title v3: Fast Refresh feat(gatsby): Set up Fast Refresh Feb 26, 2021
@wardpeet wardpeet merged commit e0bd955 into master Feb 26, 2021
@wardpeet wardpeet deleted the v3/fast-refresh branch February 26, 2021 05:58
@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.

3 participants