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

Upgrade `react-hot-loader` to 4.6 #10455

Merged
merged 9 commits into from Dec 17, 2018

Conversation

Projects
None yet
6 participants
@jgierer12
Copy link
Contributor

jgierer12 commented Dec 13, 2018

Fixes #9489

The docs also recommend (but don't require) using a patched react-dom, but as I understand it that would override the user's react-dom version we have as a peer dependency so I left it out. ➡️ 3894f0a

/cc @wKovacs64

@wKovacs64

This comment has been minimized.

Copy link
Contributor

wKovacs64 commented Dec 13, 2018

I took the Medium post and Anton's comment to mean we wouldn't need to use setConfig anymore.

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

I think we still need ignoreSFC. But you are right, we can definitely remove pureRender as that is enabled by default now.

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 13, 2018

Hmm... so this is looking good.

Unfortunately, when I test with my hooks-example, I'm getting

Unhandled Rejection (TypeError): Cannot call a class as a function

loader.getResourcesForPathname(window.location.pathname).then(() => {
  let Root = hot(module)(preferDefault(require(`./root`))) // this line
  domReady(() => {
    renderer(<Root />, rootElement, () => {
      apiRunner(`onInitialClientRender`)
    })
  })
})

I'll investigate a little further.

As info, I'm using gatsby-dev-cli as recommended in this article

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

Does flow-typed/npm/react-hot-loader_vx.x.x.js have to be updated in some way? I'm not familiar with Flow, so not sure if it should be updated automatically, manually, or not at all.

@wKovacs64

This comment has been minimized.

Copy link
Contributor

wKovacs64 commented Dec 13, 2018

I'm not able to test it myself at the moment, but it would be cool if this helps fix hooks at the top (pages) level. (Or is that more of a Gatsby internals issue?)

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

@DSchau hmm, I am getting the same error now in one of my test projects.

@wKovacs64 yes it now also works at the top level (when it works at all 😅 )

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 13, 2018

The (partial) fix is to change how we are marking the root component as hot. In src/packages/gatsby/cache-dir/app.js

we need to change line 60 from

let Root = hot(module)(preferDefault(require(`./root`)))

to

let Root = hot(preferDefault(require(`./root`)))

See the README of react-hot-loader for more info!

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 13, 2018

With the fix above (☝️) this gets us most of the way there. Nice side-benefit is that hooks seem to work in page components now, too!

Unfortunately, I seem to get this error which could be unrelated to this PR but will need to be fixed before we can merge this. Debugging that now.

screen shot 2018-12-13 at 10 51 29 am

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

@DSchau I can also reproduce that, I thought it was related to the missing react-dom patch but even with that patch I still get the same error. I will investigate further.

However, I did find a way to use the patch with a user-provided react-dom version by including RHL's webpack plugin.

@DSchau DSchau added the status: WIP label Dec 13, 2018

jgierer12 added some commits Dec 13, 2018

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

Fixed - the new hot API expects the component to be created in the parent module of where hot is called. We had it in the same module.

I will now test it with a few more projects, but the one where the two errors occurred (https://github.com/jgierer12/gatsby-api-proposal-demo) finally works now 👍

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

@DSchau what are your thoughts about #10455 (comment)?

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 13, 2018

Not sure which change caused it, but we're actually not able to hot reload at all with the most recent changes. Appears to simply refresh the page upon any content changes.

hot-reloading

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 13, 2018

@jgierer12 re:

Does flow-typed/npm/react-hot-loader_vx.x.x.js have to be updated in some way? I'm not familiar with Flow, so not sure if it should be updated automatically, manually, or not at all.

Keep it as is for now. It's relatively tangential to the PR and it appears that none of those files have been updated for the last two years 🙃

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 13, 2018

Also I'm not sure we want to implement the webpack loader as we've done in 3894f0a. I think we should prefer the babel loader, which we currently use.

This was working with the babel loader, and there's some spooky language about using the webpack plugin with class plugins.

We recommend to use babel plugin, but there are situations when you are unable to use it. Then - try webpack loader (as seen in v3), but remember - it is not compatible with class-based components, but help with TypeScript or spreading "cold API" to all node_modules.

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

Hm, for me it always hot reloads correctly. Is the repo you're testing this on public?

there's some spooky language about using the webpack plugin with class plugins

Yes, that also confused me but then again the medium article made it sound like something that's highly recommended. However, I just tried turning it off and it worked just as well.

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 13, 2018

@jgierer12 yup! https://github.com/dschau/gatsby-react-hooks/tree/4.6.0

I have gatsby-dev CLI set up to run as part of the start script, so presuming you have that setup, this will work just fine!

@jgierer12

This comment was marked as outdated.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

I'll have to call it a day for now, here are my findings in case you want to continue debugging:

  • A weird thing I noticed is that, when placing import "react-hot-loader/root" at the top of packages/gatsby/cache-dir/app.js, hot reloading works but the hot update was not successful banner is back.
  • Hot reloading seems to work fine, when the page component is either a class component or an arrow function but not a function().
@@ -57,7 +57,7 @@ apiRunnerAsync(`onClientEntry`).then(() => {
loader.addDevRequires(syncRequires)

loader.getResourcesForPathname(window.location.pathname).then(() => {
let Root = hot(module)(preferDefault(require(`./root`)))
let Root = preferDefault(require(`./root`))

This comment has been minimized.

@theKashey

theKashey Dec 13, 2018

New /root/hot API will not stand asynchronously between the first part and the second.

This comment has been minimized.

@jgierer12

jgierer12 Dec 13, 2018

Contributor

Do you mean that hot should stay in this module? i.e.

let Root = hot(preferDefault(require(`./root`)))

That was the reason for the original hot update was not successful error

This comment has been minimized.

@theKashey

theKashey Dec 13, 2018

hot or import 'react-hot-loader/root are calling module.hot.accept for the current module, thus setting self-acceptance.
It's expected that some action would take a place on module hot update immediately and in a synchronous mode. Using hot inside indirectly called function are breaking some expectations - "module" could not accept it, as long as the "real action" is not yet called.

Here - if you want to make ./root hot - you have to make IT hot. And that's was done.

The second moment is about HMR and an update propagation.
When you change a file webpack bubbles update to parent, unless parent will accept the change, then webpack will update parent and everything below it.
We had WFT-level issues if "parent" is the module with react-dom/render - everything got wiped and regenerated, making reconciliation impossible (Symbols could be updated for example).

This is not quite clear from RHL documentation, sorry. The difference is between self-accept in v4, and child-accept(ie module.hot.accept('./containers/App') in v3.

Here you might want to accept only ./root, but would accept everything.

@theKashey

This comment was marked as resolved.

Copy link

theKashey commented Dec 13, 2018

hot update was not successful would be shown when "first" part (inside /root) was called, but second (the actual hot) was not.
It's the same hot(module)(Component), just called in two steps.

Plus - @jgierer12 - pureRender is not enabled by default.

@jgierer12

This comment was marked as resolved.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

pureRender is not enabled by default

Yes, I noticed later on that I confused pureRender with pureSFC 🙈 that's why I reenabled it in 24cb4d7

@jgierer12

This comment was marked as resolved.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

It's the same hot(module)(Component), just called in two steps.

Thank you @theKashey , I realized that the first step (hot(module)) gets already called when requireing react-hot-loader/root! That helps a lot, now just to figure out how to call it synchronously.

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 13, 2018

@DSchau Could you give this another look? It now works again on my end.

@@ -19,6 +20,9 @@ setConfig({
pureRender: true,
})

const preferDefault = m => (m && m.default) || m
let Root = hot(preferDefault(require(`./root`)))

This comment has been minimized.

@theKashey

theKashey Dec 13, 2018

So - as I mentioned in another comment - hot here would setup current module as self-accepted, wiping and updating everything below it every time.

  • react-dom
  • socketIo
  • emitter

It would be better to move it to ./root or have another file, imported above this one with some important imports. That would link "important" modules with another "parent branch", and protect them during hot update.

Sound recommendation - have hot below(in module terms) react-dom/render and redux/createStore to maintain their values on update. You have to stop event propagation before it bubbles to the places you should keep between updates.

This comment has been minimized.

@jgierer12

jgierer12 Dec 13, 2018

Contributor

@theKashey thanks again, I now moved it into .cache/sync-requires.js so only page components are hot loaded.

@DSchau This is not the most beautiful solution, but it was the best I could think of without applying hot further up. An alternative could be to map over syncRequires in app.js and call hot there instead of writing it into the generated sync-requires.js.

jgierer12 added some commits Dec 13, 2018

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 14, 2018

Checking this out now! Thanks for the work on this both of you, it's appreciated!

@DSchau DSchau removed the status: WIP label Dec 14, 2018

@@ -72,15 +72,17 @@ const writePages = async () => {
components = _.uniqBy(components, c => c.componentChunkName)

// Create file with sync requires of components/json files.
let syncRequires = `// prefer default export if available
let syncRequires = `const { hot } = require("react-hot-loader/root")

This comment has been minimized.

@DSchau

DSchau Dec 14, 2018

Contributor

All of this looks great--and I was able to validate that this does fix a number of issues with hot reloading + hooks, namely:

  • Hot reloading of page components
  • Hot reloading of components in components/ directory
  • Hot reloading does not show an error on launch
  • Hot reloading (still) works with class components, markdown files, etc.

However -- I don't think this is the best place to implement this hot reloading functionality. react-hot-loader is basically a noop in non-development mode (e.g. NODE_ENV=development), but I don't love the idea of putting this in front of every page in every environment.

@pieh do you have a better idea as to where this hot reloading functionality should be implemented?

This comment has been minimized.

@jgierer12

jgierer12 Dec 14, 2018

Contributor

I agree, I originally had it in app.js (be3d53e) which also worked fine, but I moved it into the generated sync-requires.js as per #10455 (comment).

Aren't the syncRequires only loaded in development mode anyways and replaced by asyncRequires in prod?

This comment has been minimized.

@pieh

pieh Dec 16, 2018

Contributor

Aren't the syncRequires only loaded in development mode anyways and replaced by asyncRequires in prod?

Yes, they are currently - but we will be looking to split dev bundles similar to what we do for production - which should result in faster recompilations and also better experience for hosted/not-local preview servers. Not yet sure how would that affect hot reloads.

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 17, 2018

Taking one final look at this--but think we're good to go and merge this in. Will report back!

@DSchau

DSchau approved these changes Dec 17, 2018

Copy link
Contributor

DSchau left a comment

Looks good to me; merging!

@DSchau DSchau merged commit 4765c0a into gatsbyjs:master Dec 17, 2018

11 checks passed

ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsbygram Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node6 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 17, 2018

Successfully published:
 - gatsby-transformer-remark@2.1.16
 - gatsby@2.0.69

gatsby@2.0.69 should contain this updated hot reloading. Thanks all for the help and 👀 on this one! Well done!

@jgierer12 jgierer12 deleted the jgierer12:upgrade-react-hot-loader branch Dec 17, 2018

@DSchau DSchau referenced this pull request Dec 18, 2018

Closed

Gatsby not loading pages #10528

@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 18, 2018

I think this caused a fairly large regression in develop mode. Fortunately, it doesn't impact the built application, but this is still cause for concern. We'll want to get a fix out tomorrow or possibly revert this commit.

See #10528 for more info. Seems to be related to anonymous arrow functions being used as page components.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Dec 18, 2018

screen shot 2018-12-18 at 4 20 24 pm

calling `hot` for a different components is making them the same. It was designed to be applied once and to a default export only.

It would work is Component has a name, but in this case all the names are the same.

This is one line, which should be removed from RHL side, and then you will be able to use as much hots per file, as you want.

@theKashey

This comment has been minimized.

Copy link

theKashey commented Dec 18, 2018

v4.6.2 is working well on provided demo.

@jgierer12

This comment has been minimized.

Copy link
Contributor

jgierer12 commented Dec 18, 2018

@theKashey could you elaborate on what the implications are when calling hot 'further up'? After all, that's how we did it before this PR as well and it always forked fine.

I think this would be the best/only other solution without changing the underlying component structure.

Edit: Sorry, just realized you've already fixed it in 4.6.2. Thanks for the quick fix!

@theKashey

This comment has been minimized.

Copy link

theKashey commented Dec 18, 2018

The core of RHL is a "proxy" - it knows that instead of OldComponent it should use NewComponent, and that is done via top level variables registration. Basically UID for a Component is variable name + module name.
The same stuff was hidden inside hot - displayName + module name. As long as it was the same for all used components - they all become the same.

And I have no idea, why it did work before.

@google-mac

This comment has been minimized.

Copy link

google-mac commented Dec 19, 2018

React hooks are not functional with these versions:

Is there additional work to utilize the new React features in a new Gatsby project created by the CLI?

  • react@16.7.0-alpha.2
  • react-dom@16.7.0-alpha.2
  • gatsby@2.0.69
  • gatsby-transformer-remark@2.1.16
@DSchau

This comment has been minimized.

Copy link
Contributor

DSchau commented Dec 19, 2018

@google-mac can you share a repo?

That's effectively the exact same stack I'm rolling with in this repo. I just validated that gatsby develop hooks work fine, as well as in the built site

@google-mac

This comment has been minimized.

Copy link

google-mac commented Dec 20, 2018

@DSchau I just got it working in a different test. I believe there are more requirements than what I mentioned above. I looked through your repo and updated the dependencies to match for the other plugins. So it is more than React (and React-DOM), and Gatsby. Thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment