fix(gatsby): improve hot-reloading for hooks

[theKashey]

Description

This is a follow up for #13713(RHL+Hooks), and gaearon/react-hot-loader#1309 (RHL + Gatsby)

The problem: application breaking if node_modules exists in the src dir. There is no question "why", the question is "why".

Fix

In short - there is no need to apply react-hot-loader/webpack for all node_modules, only for react-dom to land the patch, and that's all the change.

Outcome:

The problem is fixed and everything is a bit faster.

Related Issues

• fix(gatsby): fix hot-reloading for hooks (patch hmr) #13713 introduced react-hot-loader/webpack
• TypeError: Object(...)(...) is not a function gaearon/react-hot-loader#1309 discovered the problem.

[pieh]

Awesome!

I do have question tho. Using node_modules in general seems like this webpack rule would potentially not be applied when working with PnP (no node_modules then).

Now this change doesn't really break it, as it would be broken before and we don't officially support PnP for now (it mostly works, but we do break it occasionally and don't have automated tests to catch regressions).

Do you feel like using a bit different rule that doesn't have notion of node_modules all together and would try to target react-dom differently, would be feasible here?

That's absolutely not blocker for PR - I'll try to merge this in tomorrow .

[theKashey]

I reckon this question should be answered by PnP itself. Many projects are expecting to find your package.json above them, many - to find .bin or node_modules, especially if they are CLI tools of some kind.
The only way to handle it is some virtualization of module and file systems, thus PnP should be somehow transparent for the target. Only time will show.

[wardpeet]

@pieh we could go for path.dirname(require.resolve('react-dom/package.json')) as our include path to fix PNP. Not sure if it's a blocker.

Thanks @theKashey for making gatsby a little bit faster

[pieh]

I'm getting React-Hot-Loader: react-🔥-dom patch is not detected. React 16.6+ features may not work. warning after applying this change:

Will try suggestion from @wardpeet

[pieh]

Nope, same warning shows up. Now I'm not sure if this is perhaps false warning - didn't try any "16.6+" features.

I added some debug logs in https://github.com/gaearon/react-hot-loader/blob/master/src/webpack/patch.js#L120-L140

And in all cases react-dom is getting patched - but there are other thing that this loader does - particularly something related to this https://github.com/gaearon/react-hot-loader/blob/master/src/webpack/webpackTagCommonJSExports.js - but I'm not quite sure what exactly that is

[pieh]

Interesting:

This is in current master:

And this is after applying either change from PR or suggestion from @wardpeet

Seems like react-dom (at least when imported in react-hot-loader is empty object) - no idea why. But react-dom actually works, because app is being mounted

[theKashey]

Ok. I know that it is - this is module execution order.
Webpack loader was not only patching react-dom, but also patching every module processed. In short it was prepending every file with import 'react-hot-loader', thus react-hot-loader would be literally the first file processed.

Before

1. React-Hot-Loader
2. React-Hot-Loader imports React-Dom
3. React-Dom imports React-Hot-Loader (circular dep, return {})
4. injected code is ok with "failed" require
5. React-Hot-Loader is finishing the initialization

Now

1. React-Dom
2. React-Dom imports React-Hot-Loader
3. React-Hot-Loader imports React-Dom (circular dep, return {})
4. RHL code is NOT ok with "failed" require
5. Patch is not landed

Solution

It's actually quite simple - load RHL before React or React-Dom. Easy to say, not so easy to do.
The common solution would be

1. just import RHL before react
2. prepend entrypoint with react-hot-loader/patch

Sorry, mate, feeling very guilty. You were lucky to detect the problem, in my case (not sure why) RHL is loading before React-Dom.

[theKashey]

Another (and quite good) solution here would be detangling RHL patch and React-Dom patches.
The problem with the second(change react-dom code) is caused by the first(add parasite import and "register" exported variables), while we need only one of these actions in a single point of time.
I would create a separate, a bit more "pure", command tomorrow. It shall solve the issue without any other hacks involved.

[pieh]

We can definitely try forcing to load RHL before react and react-dom. If this is specific to Gatsby, I don't really want to cause more work on Your part (detangling RHL and react-dom patches).

Just gonna try entry locally and report back in a second

[pieh]

Just prepending entry with this is or importing it early is causing error in gatsby:

react-lifecycles-compat.js:1 Uncaught TypeError: Cannot read property 'signature' of undefined
    at Object../.cache/react-lifecycles-compat.js (react-lifecycles-compat.js:1)
    at __webpack_require__ (bootstrap:723)
    at fn (bootstrap:100)
    at Object../node_modules/react-hot-loader/dist/react-hot-loader.development.js (react-hot-loader.development.js:13)
    at __webpack_require__ (bootstrap:723)
    at fn (bootstrap:100)
    at Object../node_modules/react-hot-loader/patch.js (patch.js:6)
    at __webpack_require__ (bootstrap:723)
    at fn (bootstrap:100)
    at Object.0 (page-2.js:1)

But we do have some weird thing:

gatsby/packages/gatsby/src/utils/webpack.config.js

Lines 383 to 385 in 02f576e

	"react-lifecycles-compat": directoryPath(
	`.cache/react-lifecycles-compat.js`
	),

https://github.com/gatsbyjs/gatsby/blob/c19a9c7c45c404b9008dba7e5bb33a25aff0ccaf/packages/gatsby/cache-dir/react-lifecycles-compat.js
That was added in #7214

Removing that alias, fixes it, but I lack context on that. I'm weary of just removing it, so probably I would try to see if I can make it work with adding more "passthrough" stuff to our custom react-lifecycles-compat

[pieh]

/***/ "./.cache/react-lifecycles-compat.js":
/*!*******************************************!*\
  !*** ./.cache/react-lifecycles-compat.js ***!
  \*******************************************/
/*! no static exports found */
/***/ (function(module, exports) {

var __signature__=typeof reactHotLoaderGlobal!=='undefined'?reactHotLoaderGlobal.default.signature:function(a){return a;};exports.polyfill=function(Component){return Component;};

/***/ }),

This is where it crashes ( trying to access reactHotLoaderGlobal.default.signature ). It does seem similar to react-dom being empty object:

I guess this happens because RHL itself imports react-lifecycles-compat. I do wonder however why this doesn't happen when using regular react-lifecycles-compat and not our passthrough hack

[pieh]

Blacklisting react-lifecycles-compat from any of our babel loaders (that use react-hot-loader/babel babel plugin) seems to do the trick here, but this is getting pretty messy ;)

[theKashey]

You are the real Sherlock, and look like I have think a bit harder about module resolution order.

• you are importing RHL
• RHL is importing react-lifecycles-compat
• react-lifecycles-compat is importing RHL, (circular dep, return {})
• 💥

It should be or more resilient, or ignore all own dependencies. I would try to reproduce the worst scenario and make it work without any extra configuration. Honestly - it should not require any extra integration steps. Let's take a pause and not make it worse 😅

[pieh]

I guess this happens because RHL itself imports react-lifecycles-compat. I do wonder however why this doesn't happen when using regular react-lifecycles-compat and not our passthrough hack

After few more tests I can answer this at least - when not using alias, our webpack config doesn't use babel-loader (that includes react-hot-loader/babel) on packages inside node_modules (our aliased react-lifecycles-compat is being copied outside of node_modules, so it's handled by babel-loader).

Honestly - it should not require any extra integration steps. Let's take a pause and not make it worse 😅

Yeah, I agree - we can make it work, but amount of hackiness we'd have to do make me pause.

It's also interesting how accidentally current setup workarounds circular imports issue right now ;)

[theKashey]

So - I've failed to solve the problem with react-lifecycles-compat, and thus I am going to attack this problem from another angle and remove require("react-hot-loader") injections - so the loops would not ever occur. Let's end this.

And thank you very much for guiding me to this decision.

[sidharthachatterjee]

Thank you so much for working on this @theKashey

This indeed does seem to be a little tricky and since it appears that we need to rethink how to approach this/discuss some more, I think we should close this for the moment.

I've opened an issue in #16534 to track this! Shall we continue the discussion there?

Thank you so much once again for your work on this! We really appreciate it and would love to have you as a contributor so we're looking forward to receiving more PRs from you 💜

[theKashey]

So actually all changes for this issue, including support for React 16.9, were merged tomorrow. I am doing last manual checks with a few selected projects, and releasing a new version today.

Then this PR, amended with a version bump, should work almost out of the box (at last!)

[sidharthachatterjee]

@theKashey Awesome! Feel free to reopen this with the version bump when you're ready

[theKashey]

The PR has been updated, however I could not reopen this one without your help, and it would not reflect the changes made while closed.

[sidharthachatterjee]

It says the master branch was force pushed or re created so it won't let me re open either! 😅 Let's open a new one and get it in? 💃 Thank you!