-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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(hmr): accept hot updates for modules above page templates #29752
Changes from all commits
6184a52
badc14d
5bbb990
547823d
0a24e00
dd7ce67
5f2246c
168a79c
ecc7183
555b9ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
const TEST_ID = `gatsby-browser-hmr` | ||
|
||
describe(`hot reloading above page template (gatsby-browser)`, () => { | ||
beforeEach(() => { | ||
cy.visit(`/`).waitForRouteChange() | ||
}) | ||
it(`displays placeholder content on launch`, () => { | ||
cy.getTestElement(TEST_ID).should( | ||
`contain.text`, | ||
`%TEST_HMR_IN_GATSBY_BROWSER%` | ||
) | ||
}) | ||
|
||
it(`hot reloads with new content`, () => { | ||
const text = `HMR_IN_GATSBY_BROWSER_WORKS` | ||
cy.exec( | ||
`npm run update -- --file src/wrap-root-element.js --replacements "TEST_HMR_IN_GATSBY_BROWSER:${text}"` | ||
) | ||
|
||
cy.waitForHmr() | ||
|
||
cy.getTestElement(TEST_ID).should(`contain.text`, text) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,16 +13,15 @@ import asyncRequires from "$virtual/async-requires" | |
// Generated during bootstrap | ||
import matchPaths from "$virtual/match-paths.json" | ||
import { LoadingIndicatorEventHandler } from "./loading-indicator" | ||
|
||
import Root from "./root" | ||
import { init as navigationInit } from "./navigation" | ||
// ensure in develop we have at least some .css (even if it's empty). | ||
// this is so there is no warning about not matching content-type when site doesn't include any regular css (for example when css-in-js is used) | ||
// this also make sure that if all css is removed in develop we are not left with stale commons.css that have stale content | ||
import "./blank.css" | ||
|
||
// Enable fast-refresh for virtual sync-requires | ||
module.hot.accept(`$virtual/async-requires`, () => { | ||
// Manually reload | ||
}) | ||
// Enable fast-refresh for virtual sync-requires and gatsby-browser | ||
module.hot.accept([`$virtual/async-requires`, `./api-runner-browser`]) | ||
|
||
window.___emitter = emitter | ||
|
||
|
@@ -160,8 +159,8 @@ apiRunnerAsync(`onClientEntry`).then(() => { | |
loader.loadPage(`/404.html`), | ||
loader.loadPage(window.location.pathname), | ||
]).then(() => { | ||
const preferDefault = m => (m && m.default) || m | ||
const Root = preferDefault(require(`./root`)) | ||
Comment on lines
-163
to
-164
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was causing some weird module dependency chains and it wasn't exactly playing nice with HMR - hot updates for gatsby-browser modules were declined even tho I was accepting them on line 23 (for I'm not exactly sure why it was done this way (delayed commonjs import) :/ |
||
navigationInit() | ||
|
||
domReady(() => { | ||
if (dismissLoadingIndicator) { | ||
dismissLoadingIndicator() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,19 +2,13 @@ import React from "react" | |
import { Router, Location, BaseContext } from "@reach/router" | ||
import { ScrollContext } from "gatsby-react-router-scroll" | ||
|
||
import { | ||
shouldUpdateScroll, | ||
init as navigationInit, | ||
RouteUpdates, | ||
} from "./navigation" | ||
import { shouldUpdateScroll, RouteUpdates } from "./navigation" | ||
import { apiRunner } from "./api-runner-browser" | ||
import loader from "./loader" | ||
import { PageQueryStore, StaticQueryStore } from "./query-result-store" | ||
import EnsureResources from "./ensure-resources" | ||
import FastRefreshOverlay from "./fast-refresh-overlay" | ||
|
||
navigationInit() | ||
|
||
// In gatsby v2 if Router is used in page using matchPaths | ||
// paths need to contain full path. | ||
// For example: | ||
|
@@ -103,7 +97,7 @@ const Root = () => ( | |
) | ||
|
||
// Let site, plugins wrap the site e.g. for Redux. | ||
const WrappedRoot = apiRunner( | ||
const rootWrappedWithWrapRootElement = apiRunner( | ||
`wrapRootElement`, | ||
{ element: <Root /> }, | ||
<Root />, | ||
|
@@ -112,8 +106,12 @@ const WrappedRoot = apiRunner( | |
} | ||
).pop() | ||
|
||
export default () => ( | ||
<FastRefreshOverlay> | ||
<StaticQueryStore>{WrappedRoot}</StaticQueryStore> | ||
</FastRefreshOverlay> | ||
) | ||
function RootWrappedWithOverlayAndProvider() { | ||
return ( | ||
<FastRefreshOverlay> | ||
<StaticQueryStore>{rootWrappedWithWrapRootElement}</StaticQueryStore> | ||
</FastRefreshOverlay> | ||
) | ||
} | ||
|
||
export default RootWrappedWithOverlayAndProvider | ||
Comment on lines
+109
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name our Root Component :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually important - whatever we pass to
wrapRootElement
(orwrapPageElement
) is NOT treated as component. We just straight call this API (and not useReact.createElement
) - that's because this is gatsby-browser API hook and we also pass pluginOptions as second parameter. Because of above this is actually not represented as React Component in react tree and react-refresh won't actually apply HMR to it (even tho webpack plugin would treat Wrapper as react component and register it as such). On top of problems there are multiple exports ingatsby-browser
and that is also no-no for react-refresh.Above is also reason why we can't use hooks in there etc
This might need some additional notes in https://www.gatsbyjs.com/docs/reference/config-files/gatsby-browser/#wrapPageElement (and maybe migration guide?)