RHL throwing ReferenceErrors when hot-reloading third-party hooks. #1268

phyllisstein · 2019-06-15T10:35:39Z

Description

👋 Hey folks! I'm having a little trouble with RHL's hooks support in a little Gatsby project. RHL seems to be injecting code that evaluates the imported hook functions using their reference names in the source code. But since the imports have been transformed by Babel and/or Webpack, there's nothing in scope that matches. React's built-in hooks do not seem to be affected.

By way of a contrived example, here's a silly SFC that uses react-spring:

import { animated, useSpring } from 'react-spring'
import React, { useCallback, useState } from 'react'

function Index() {
  const [thingDone, toggleThingDone] = useState(false)
  const doTheThing = useCallback(() => toggleThingDone(!thingDone), [thingDone])

  const fader = useSpring({ opacity: thingDone ? 1 : 0 })

  return (
    <>
      <animated.h1 style={ fader }>
        You did the thing!
      </animated.h1>
      <button type='button' onClick={ doTheThing }>
        { thingDone ? 'Undo The Thing' : 'Do The Thing' }
      </button>
    </>
  )
}

I dropped the transpiled code into this gist; the relevant hunks seem to be line 22:

var _reactSpring = __webpack_require__(/*! react-spring */ "./node_modules/react-spring/web.js");

...lines 42--44:

const fader = (0, _reactSpring.useSpring)({
  opacity: thingDone ? 1 : 0
});

...and line 63:

__signature__(Index, "useState{[thingDone, toggleThingDone]}\nuseCallback{doTheThing}\nuseSpring{fader}", () => [useSpring]);

The component uses the transpiled reference as expected, but RHL doesn't seem to know about it. When a hot update is triggered, it throws a ReferenceError:

Stack trace 💥

Uncaught ReferenceError: useSpring is not defined at Object.getCustomHooks (index.js:5) at haveEqualSignatures (react-hot-loader.development.js:2349) at areSignaturesCompatible (react-hot-loader.development.js:2370) at compareRegistered (react-hot-loader.development.js:2384) at compareComponents (react-hot-loader.development.js:2395) at hotComponentCompare (react-hot-loader.development.js:2468) at reconcileSingleElement (react-dom.development.js:12507) at reconcileChildFibers (react-dom.development.js:12589) at reconcileChildren (react-dom.development.js:14411) at finishClassComponent (react-dom.development.js:14759) at updateClassComponent (react-dom.development.js:14697) at beginWork (react-dom.development.js:15645) at performUnitOfWork (react-dom.development.js:19313) at workLoop (react-dom.development.js:19353) at renderRoot (react-dom.development.js:19436) at performWorkOnRoot (react-dom.development.js:20343) at performWork (react-dom.development.js:20255) at performSyncWork (react-dom.development.js:20229) at requestWork (react-dom.development.js:20098) at scheduleWork (react-dom.development.js:19912) at Object.enqueueForceUpdate (react-dom.development.js:11209) at ExportedComponent../node_modules/react/cjs/react.development.js.Component.forceUpdate (react.development.js:353) at react-hot-loader.development.js:2740 at Array.forEach () at react-hot-loader.development.js:2739 (anonymous) @ index.js:5 haveEqualSignatures @ react-hot-loader.development.js:2349 areSignaturesCompatible @ react-hot-loader.development.js:2370 compareRegistered @ react-hot-loader.development.js:2384 compareComponents @ react-hot-loader.development.js:2395 hotComponentCompare @ react-hot-loader.development.js:2468 reconcileSingleElement @ react-dom.development.js:12507 reconcileChildFibers @ react-dom.development.js:12589 reconcileChildren @ react-dom.development.js:14411 finishClassComponent @ react-dom.development.js:14759 updateClassComponent @ react-dom.development.js:14697 beginWork @ react-dom.development.js:15645 performUnitOfWork @ react-dom.development.js:19313 workLoop @ react-dom.development.js:19353 renderRoot @ react-dom.development.js:19436 performWorkOnRoot @ react-dom.development.js:20343 performWork @ react-dom.development.js:20255 performSyncWork @ react-dom.development.js:20229 requestWork @ react-dom.development.js:20098 scheduleWork @ react-dom.development.js:19912 enqueueForceUpdate @ react-dom.development.js:11209 ./node_modules/react/cjs/react.development.js.Component.forceUpdate @ react.development.js:353 (anonymous) @ react-hot-loader.development.js:2740 (anonymous) @ react-hot-loader.development.js:2739 setTimeout (async) updateInstances @ react-hot-loader.development.js:2732 (anonymous) @ react-hot-loader.development.js:2756 hotSetStatus @ bootstrap:246 hotApply @ bootstrap:628 cb @ process-update.js:76 (anonymous) @ process-update.js:91 Promise.then (async) check @ process-update.js:90 ./node_modules/webpack-hot-middleware/process-update.js.module.exports @ process-update.js:52 processMessage @ client.js:279 handleMessage @ client.js:139 handleMessage @ client.js:102 06:17:01.939 index.js:2177 The above error occurred in the component: in LocationProvider (created by Context.Consumer) in Location (created by Context.Consumer) in Router (created by Root) in Root (at root.js:90) in ThemeProvider (at provider.js:7) in Provider (at gatsby-browser.js:21) in _default (at app.js:62)

Consider adding an error boundary to your tree to customize error handling behavior. Visit https://fb.me/react-error-boundaries to learn more about error boundaries. stack_frame_overlay_proxy_console @ index.js:2177 logCapturedError @ react-dom.development.js:17118 logError @ react-dom.development.js:17154 update.callback @ react-dom.development.js:18066 callCallback @ react-dom.development.js:16434 commitUpdateEffects @ react-dom.development.js:16473 commitUpdateQueue @ react-dom.development.js:16461 commitLifeCycles @ react-dom.development.js:17384 commitAllLifeCycles @ react-dom.development.js:18737 callCallback @ react-dom.development.js:150 invokeGuardedCallbackDev @ react-dom.development.js:200 invokeGuardedCallback @ react-dom.development.js:257 commitRoot @ react-dom.development.js:18949 (anonymous) @ react-dom.development.js:20419 unstable_runWithPriority @ scheduler.development.js:255 completeRoot @ react-dom.development.js:20418 performWorkOnRoot @ react-dom.development.js:20347 performWork @ react-dom.development.js:20255 performSyncWork @ react-dom.development.js:20229 requestWork @ react-dom.development.js:20098 scheduleWork @ react-dom.development.js:19912 enqueueForceUpdate @ react-dom.development.js:11209 ./node_modules/react/cjs/react.development.js.Component.forceUpdate @ react.development.js:353 (anonymous) @ react-hot-loader.development.js:2740 (anonymous) @ react-hot-loader.development.js:2739 setTimeout (async) updateInstances @ react-hot-loader.development.js:2732 (anonymous) @ react-hot-loader.development.js:2756 hotSetStatus @ bootstrap:246 hotApply @ bootstrap:628 cb @ process-update.js:76 (anonymous) @ process-update.js:91 Promise.then (async) check @ process-update.js:90 ./node_modules/webpack-hot-middleware/process-update.js.module.exports @ process-update.js:52 processMessage @ client.js:279 handleMessage @ client.js:139 handleMessage @ client.js:102

I'm willing to believe this is an issue with Gatsby's configuration patterns or my particular abuses of Gatsby's configuration patterns, so I'm working on putting together a lightweight reproduction case. But I wanted to run it by you folks in the meantime to see whether there were any known issues that may be germane.

Let me know if I can do anything to add more color to the problem or help out with a solution! Thanks so much for your insight. ✨

Expected behavior

RHL should invoke hooks with their transpiled references, so as to avoid throwing ReferenceErrors.

Actual behavior

RHL attempts to invoke hooks using their untranspiled references, which are never in scope and throw ReferenceErrors.

Environment

React Hot Loader version: v4.11.0

Run these commands in the project folder and fill in their results:

node -v: v12.4.0
npm -v: v6.9.0
yarn -v': v1.16.0

Then, specify:

Operating system: macOS 10.15 (19A471t)
Browser and version: Chrome 76.0.3809.21

Reproducible Demo

🚧 So far just the gist, but I'm working on it!

The text was updated successfully, but these errors were encountered:

theKashey · 2019-06-15T11:47:49Z

Got the point, and I know that's the problem.

theKashey · 2019-06-15T12:24:29Z

I recon this is related to facebook/react#15883, backporting to RHL...

theKashey · 2019-06-15T13:22:59Z

v.4.11.1 released. Containing fixes both for your problem and react-spring.
https://github.com/gaearon/react-hot-loader/releases/tag/v4.11.1

phyllisstein · 2019-06-16T05:38:58Z

Thank you for pouncing on this so quickly! (And, incidentally, for the honor of adding that little code snippet to the official examples. ✨) I gave the updated version a shot, and the results were mixed: upgrading did not help with my issue, but I had misdiagnosed it anyway; once I landed on the actual problem, the fix was straightforward.

The bug is indeed triggered when reloading a module with hooks---but only if Babel's CommonJS transform is enabled. Builds that allow import/export statements to fall through to Webpack work fine; if Babel gets there first, the references fall out of sync and hot reloads throw an error. I hammered out a super lightweight test case based on your prior art in the Styled Components example; there's some prebuilt code in there as well.

My immediate error, for what little it's worth, was fat-fingering my babel.conf.js such that Webpack never even had a chance:

module.exports = api => {
-  const isWebpackBuild = api.caller(caller => !!(caller && caller.name === 'babel-webpack'))
+  const isWebpackBuild = api.caller(caller => !!(caller && caller.name === 'babel-loader'))

  return {
    presets: [
      ['@babel/env', {
        modules: isWebpackBuild ? false : 'commonjs',
      }],
    ],
  }
}

Given how rare it is to see folks preferring Babel's modules to Webpack's at this point, this doesn't seem especially pressing, at least from where I sit. So unless a fix springs to mind immediately, I'd be happy to dig into RHL's recent evolution and see whether I can make any headway. I'm sure it'd take a minute to get caught up, but reading this code has always been a learning experience.

theKashey · 2019-06-16T07:52:57Z

Ok, not the problem is more clear, and thank you for the simple way to cause it.
Unfortunately, now I am not quite sure how to fix it - solution pre tree traversal always worked before.

theKashey · 2019-06-16T09:46:48Z

I've tried babel 6 and babel 7 both unit tests and examples, with different module values. It always works.
I could not reproduce the problem

phyllisstein · 2019-06-21T23:18:58Z

You're right... I can reproduce, but not consistently. I must have stumbled over some kind of incompatibility, but it's not as simple as activating CommonJS. I'll keep prodding and narrow it down.

dgreensp · 2019-06-29T03:08:37Z

I'm hitting the same problem, with v4.11.1, using Parcel (TypeScript + Babel).

theKashey · 2019-06-29T03:48:47Z

The problem is understandable - two transforms are fighting with each other, but I am not sure how to fix it, as long as the best known solution was already applied.

dgreensp · 2019-06-29T07:49:42Z

I dug into the problem, and I think I have an idea what's going on, and a potential fix.

The reason plugin-transform-modules-commonjs isn't rewriting both occurrences of the custom hook identifier (useSpring in the example) is because they are the same node object, so it thinks it's already done it. That is, the react-hot-loader transform puts the same node object in two different places in the tree; the module transformer encounters one and replaces it by path, and then ignores the other when it visits it, because it's already seen that node.

A solution is to change this line

react-hot-loader/src/fresh/babel.js

Line 205 in 70353e5

.map(call => call.callee),

to say:

.map(call => t.cloneNode(call.callee)),

I haven't written a lot of Babel plugins or anything, but I think a deep clone is appropriate, because call.callee could be a MemberExpression, and we don't want to run into the same problem where the foo in foo.useBar is not rewritten to _myModule.foo.

theKashey · 2019-06-29T08:18:01Z

I think the better way would be to remove checks after, as long as the tree is mutating in an exclusive way, and no other plugins could interfere, ie if (seenForSignature.has(node)) {.

@gaearon - you should have this problem as well.

dgreensp · 2019-06-29T08:36:41Z

@theKashey we might be not understanding each other.

The code that thinks it has already seen the node is not part of react-hot-loader, it is in Babel: https://github.com/babel/babel/blob/1d3f9815df747902ae535706f032081bc4825b87/packages/babel-helper-module-transforms/src/rewrite-live-references.js#L169

To give an example of the problem, if you imagine a transform that turns this:

import { foo } from 'bar'

foo()

into this:

import { foo } from 'blah'

foo()
foo()

then it seems important that the newly inserted foo identifier is a new, different node object. Otherwise, Babel will turn it into something like:

var _blah = require('blah')

_blah.foo()
foo()

... which is clearly wrong. Babel is not expecting the same node at two different paths.

theKashey · 2019-06-29T08:38:10Z

@dgreensp - You are right, I got your message not correctly, and understand what did you mean only later. I've tried to create any red test, using a simple repetition like you proposed, but failed - always working for me.

theKashey · 2019-06-29T08:39:14Z

Ie - we could fix in a way you proposed, but how to verify?

dgreensp · 2019-06-29T08:43:25Z

Hmm, what are the current tests relevant to the babel plugin? The test that's needed is basically that the react-hot-loader babel plugin composes correctly with the import/export plugin in the case of custom hooks. Specifically, a custom hook imported from another module.

theKashey · 2019-06-29T08:49:11Z

@dgreensp - see the commit ^above^. Unfortunately, the snapshot is always correct. However - that's also not bad - we could just shit fix and hope that it would help.

dgreensp · 2019-06-29T10:03:06Z

I'll take a look and see if I can understand why it is passing. Maybe a different plugin order or something.

theKashey · 2019-06-29T10:05:24Z

@phyllisstein example is also always working for me. Could it be the case that you both have some unlucky package cached (should not be possible with yarn.lock, but 🤷‍♂️)

dgreensp · 2019-06-29T15:57:07Z

I cloned the repo and am trying to run the tests. I'm currently stuck on a failure just running the tests as-is on master: Cannot find module './react-dom' from 'react-dom.integration.spec.js'.

dgreensp · 2019-06-29T16:00:58Z

Never mind, I ran yarn run test:react-dom:prepare and it fixed it.

dgreensp · 2019-06-29T16:33:40Z

Ok, the reason the tests aren't failing is because Babel 6 has different transformation code from Babel 6. The relevant Babel 6 code is here: https://github.com/babel/babel/blob/ffcd8d85e4bdc271a0390596f88a33e2218e5925/packages/babel-plugin-transform-es2015-modules-commonjs/src/index.js#L54 . It doesn't mind transforming two nodes that are === each other, whereas Babel 7 does.

Is there a way to add Babel 7 tests?

Note also:

This has nothing to do with whether you use a hook more than once. The second occurrence that doesn't get transformed is in generated code:

// generated code by react-hot-loader plugin
__signature__(
  useCustomHook,
  'useState{(42)}\\\\nuseEffectHook{}\\\\nuseExternalHook{}',
  function () {
    return [
      useEffectHook,
      _externalHook.useExternalHook // this is the occurrence that doesn't get transformed by Babel 7
    ];
  }
);

I believe the appropriate clone function is t.cloneNode, because it's deep, whereas t.clone is shallow. An additional test that does import { myHooks } followed by myHooks.useAnotherExternalHook should confirm this, once we have a failing test for the plain external hook case.

dgreensp · 2019-06-29T16:45:21Z

Now trying to run an example that uses Babel 7. The instructions to run yarn install file:../packages/react-hot-loader to hook up an example to the locally built react-hot-loader seem to be out of date and aren't working.

dgreensp · 2019-06-29T17:50:20Z

I've reproduced the problem by modifying an example app. None of the example apps quite have a suitable config as-is, because to run into the problem you need a combination of:

Babel 7
React with Hooks
Appropriately patched react-dom
Babel transforming the import/export syntax, not Webpack 2, which can be achieved by using Parcel, or in a Webpack app by explicitly telling Babel to transform modules to commonjs

This is mostly just a matter of using the latest and greatest stuff; none of these elements are unusual, of course.

I've made some modifications to the Parcel example to reproduce the problem. Let's see if I can get it on a branch or something for ease of collaboration.

dgreensp · 2019-06-29T18:07:12Z

Ok, here is a repro: master...dgreensp:repro-hook-issue . This is all I have time to do for today, probably.

When messing with package.json and stuff in a Parcel project, make sure you clear the cache. I would run rm -rf .cache && yarn start to start the server each time I want to try it.

With these changes, just saving the file Problem.js in my editor causes a crash. Here is the money shot:

Anyway, I think the easiest way to test for it is to have the tests run Babel 7 (somehow, ideally in addition to 6) and confirm that the output is transformed correctly.

dgreensp · 2019-06-29T18:09:50Z

I created a PR for the repro in case that makes it easier to check out.

theKashey · 2019-06-29T23:16:51Z

🪨

Reproduce #1268

theKashey · 2019-06-29T23:44:59Z

t.clone confirmed to fix the problem.

fix: clone node for signature, fixes #1268

theKashey · 2019-06-30T05:25:01Z

v4.11.2, or, better, v4.12 should solve the problem

phyllisstein · 2019-06-30T12:20:56Z

Thanks for connecting the dots here and coming up with a patch!

theKashey · 2019-07-02T23:51:01Z

😅 look like the fix was not properly released :)

phyllisstein · 2019-07-03T00:07:29Z

phyllisstein changed the title ~~RHL throwing ReferenceErrors when hot-reloading hooks.~~ RHL throwing ReferenceErrors when hot-reloading third-party hooks. Jun 15, 2019

theKashey self-assigned this Jun 15, 2019

theKashey added the help wanted label Jun 16, 2019

theKashey added a commit that referenced this issue Jun 29, 2019

fix: clone node for signature, fixes #1268

57d0043

theKashey mentioned this issue Jun 29, 2019

fix: clone node for signature, fixes #1268 #1276

Merged

theKashey added a commit that referenced this issue Jun 29, 2019

fix: clone node for signature, fixes #1268

ed3e1d9

dgreensp mentioned this issue Jun 29, 2019

Reproduce #1268 #1278

Merged

theKashey added a commit that referenced this issue Jun 29, 2019

Merge pull request #1278 from dgreensp/repro-hook-issue

bc99cf6

Reproduce #1268

theKashey closed this as completed in #1276 Jun 30, 2019

theKashey added a commit that referenced this issue Jun 30, 2019

Merge pull request #1276 from gaearon/fix-common-signature

426281b

fix: clone node for signature, fixes #1268

theKashey added a commit that referenced this issue Jun 30, 2019

make parcel example more fragile to #1268

3cb9c64

theKashey mentioned this issue Jun 30, 2019

[Fresh] Clone a custom hook node before use facebook/react#16019

Merged

theKashey mentioned this issue Jul 2, 2019

Moving hook call to another module and importing result breaks RHL #1280

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RHL throwing ReferenceErrors when hot-reloading third-party hooks. #1268

RHL throwing ReferenceErrors when hot-reloading third-party hooks. #1268

phyllisstein commented Jun 15, 2019 •

edited

theKashey commented Jun 15, 2019

theKashey commented Jun 15, 2019

theKashey commented Jun 15, 2019

phyllisstein commented Jun 16, 2019

theKashey commented Jun 16, 2019

theKashey commented Jun 16, 2019

phyllisstein commented Jun 21, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019 •

edited

theKashey commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019 •

edited

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

theKashey commented Jun 29, 2019

theKashey commented Jun 30, 2019

phyllisstein commented Jun 30, 2019

theKashey commented Jul 2, 2019

phyllisstein commented Jul 3, 2019

Navigation Menu

RHL throwing ReferenceErrors when hot-reloading third-party hooks. #1268

RHL throwing ReferenceErrors when hot-reloading third-party hooks. #1268

Comments

phyllisstein commented Jun 15, 2019 • edited

Description

Expected behavior

Actual behavior

Environment

Reproducible Demo

theKashey commented Jun 15, 2019

theKashey commented Jun 15, 2019

theKashey commented Jun 15, 2019

phyllisstein commented Jun 16, 2019

theKashey commented Jun 16, 2019

theKashey commented Jun 16, 2019

phyllisstein commented Jun 21, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019 • edited

theKashey commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019 • edited

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

dgreensp commented Jun 29, 2019

theKashey commented Jun 29, 2019

theKashey commented Jun 29, 2019

theKashey commented Jun 30, 2019

phyllisstein commented Jun 30, 2019

theKashey commented Jul 2, 2019

phyllisstein commented Jul 3, 2019

phyllisstein commented Jun 15, 2019 •

edited

dgreensp commented Jun 29, 2019 •

edited

dgreensp commented Jun 29, 2019 •

edited