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

AppContainer is using ReactNoopUpdateQueue in dev builds, preventing React component state updates #593

Closed
TAGC opened this issue Jun 20, 2017 · 18 comments

Comments

@TAGC
Copy link

TAGC commented Jun 20, 2017

Description

I'm facing a really strange issue in which every now and again my app stops working when running via webpack's hot dev server. When this issue presents itself, I struggle to get it to work again even after cleaning everything, stashing all git changes, resetting yarn.lock, etc.

In production builds I noticed this issue never occurs.

I have a simple component that renders two fields ("hostname" and "port"), and maintains the field values in the component's state. Essentially just this.

Expected behavior

I expect in such a basic case that I should be able to update the form fields without issue - I enter text in a field, which triggers the field's onChange event, which in turn invokes setState, updating the value that
the field gets re-rendered with.

Actual behavior

I observe the expected behaviour every time in production, and some of the time during development. I have no idea why but sometimes after rebuilding certain modules I will see this issue again.

When it does occur, I'm not able to change the value of either of the fields, and instead I see the following error in the Electron console:

warning.js:36 Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the ConnectionPane component.

Environment

React Hot Loader version: 3.0.0-beta.7

  1. node -v: v7.7.2

  2. npm -v: 5.0.3

  3. Operating system: Windows 7 64-bit

  4. Browser and version: Electron (v1.7.3) chromium browser

Additional details

I traced the immediate cause of the issue.

The problem starts when I render my React tree right at the top-level in index.tsx: render(() => Routing(history));

Through a long chain of calls through the React (v15.6.1) library, we get to this point:

  _constructComponentWithoutOwner: function (doConstruct, publicProps, publicContext, updateQueue) {
    var Component = this._currentElement.type;

    if (doConstruct) {
      if (process.env.NODE_ENV !== 'production') {
        return measureLifeCyclePerf(function () {
          return new Component(publicProps, publicContext, updateQueue);
        }, this._debugID, 'ctor');
      } else {
        return new Component(publicProps, publicContext, updateQueue);
      }
    }

...where Component is AppContainer.

The React library passes the props, context and update queue to AppContainer. However, AppContainer's constructor completely ignores the latter two of these arguments:

  function AppContainer(props) {
    _classCallCheck(this, AppContainer);

    var _this = _possibleConstructorReturn(this, (AppContainer.__proto__ || Object.getPrototypeOf(AppContainer)).call(this, props));

    _this.state = { error: null };
    return _this;
  }

This means that a little further up, we get this.updater = updater || ReactNoopUpdateQueue, where updater is undefined (because AppContainer dropped it), and so it gets set to the noop update queue which leads to the issue I'm seeing.

This constructor is specifically for AppContainer.dev.js; the production version has a different constructor that doesn't lead to this issue, which is why my production builds are fine.

Here's an image of the top part of the callstack:

callstack

@TAGC
Copy link
Author

TAGC commented Jun 20, 2017

Actually sorry, I don't think the initialisation of AppContainer is relevant. It's pretty normal for React components to ignore those last two arguments. I guess AppContainer just jumped out at me because it's the first container to get instantiated (i.e. first to trip the breakpoint), and the way it ignores those two arguments initially looks like a bug.

I should be looking at the same callstack except for when my own ConnectionPane component is being constructed, not AppContainer.

@TAGC
Copy link
Author

TAGC commented Jun 20, 2017

I'll close this for now because I don't think it's a react-hot-loader issue.

@TAGC TAGC closed this as completed Jun 20, 2017
@rosskevin
Copy link

rosskevin commented Jun 22, 2017

@TAGC I'm encountering the same error with 3.0.0-beta.7.

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component.

My component is confirmed mounted (it would be impossible at the root to encounter this situation).

I have meticulously compared to react-hot-boilerplate#next and do not spot an error. The moment I disable hot loading, my app works.

 node -v                                                                                                                                                                                                            develop ⬆ ⬇ ✱
v8.1.2
~/p/c/ui ❯❯❯ yarn list webpack webpack-dev-server react-hot-loader                                                                                                                                                              develop ⬆ ⬇ ✱
yarn list v0.24.6
├─ react-hot-loader@3.0.0-beta.7
├─ webpack-dev-server@2.5.0
└─ webpack@3.0.0

I think this is a bug. I can file another issue, or you could reopen this one.

@rosskevin
Copy link

rosskevin commented Jun 22, 2017

Similar symptom #446, #457

@rosskevin
Copy link

Possibly related to #313

My .babelrc is:

{
  "sourceMaps": "inline",
  "presets": [
    [
      "env",
      {
        "targets": {
          "browsers": "> 5%"
        },
        "modules": false,
        "debug": true
      }
    ],
    "stage-2",
    "react"
  ],
  "plugins": [
    "react-hot-loader/babel",
    ["relay", {"compat": true, "schema": "./schema.json"}],
    "transform-flow-strip-types",
    ["lodash", { "id": ["lodash", "recompose"] }]
  ]
}

@TAGC TAGC reopened this Jun 22, 2017
@ryancole
Copy link

ryancole commented Jun 23, 2017

I'm also experiencing this issue, using beta 7.

I don't have my project handy at the moment to rattle off version numbers, but they're likely similar to the others in this thread. I just wanted to reply to add to the list of folks experiencing the issue.

Edit: It's definitely something related to react hot loader. I tried using specific, older versions of the beta and the error exists at all versions. If I remove react hot loader entirely everything works as expected. After looking at some of the release notes related to the beta releases, react hot loader appears to do a lot of things related to babel transforms and class properties, etc. There's just most likely some clash between what react hot loader is applying and what babel intends, or what react expects, etc.

@TAGC
Copy link
Author

TAGC commented Jun 23, 2017

Worked around this issue as I commented in facebook/react#10012.

@rosskevin
Copy link

rosskevin commented Jun 26, 2017

I have tried all workarounds in linked issues with no success, including adding transform-es2015-classes to my .babelrc. I have not tried to use bind in the constructor because that would be a massive undertaking for my codebase.

Using the transform-es2015-classes creates errors for me such as Class constructor RouteConfig cannot be invoked without 'new'.

@torrac12
Copy link

torrac12 commented Jul 3, 2017

I fixed it with add transform-es2015-classes in my .babelrc.
I used to use babel-loader handle es6 compile,and there is no error. When I remove it,it warning can not setState on a unmount component。And when i import transform-es2015-classes, it works again. So is there difference between the native es6 class from babel transformed class?

@bspies-work
Copy link

bspies-work commented Aug 1, 2017

Running into the same issue in our code. Have a simple input field for a zip code with an event handler like so:

handleChange = (value) => {
     if (!this.state.zipChanged) {
          this.setState({ zipChanged: true });
     }
}

Removing 'react-hot-loader/babel' from the babel loader fixes the issue, but breaks hot reloading. We are using the react, env, stage-2 presets, and transform-decorators-legacy plugin with the babel (v 6.24.1) loader. Env gives us:
transform-es2015-function-name {"edge":15}
transform-exponentiation-operator {"ios":10}
transform-async-to-generator {"ios":10}

I have created a simpler project that reproduces this issue here: https://github.com/bspies-work/react-hot-load

@noinkling
Copy link

My temporary solution has been to remove react-hot-loader/patch from my webpack entrypoint (and leave everything else as-is). As far as I can tell, hot-reloading still works, it just doesn't retain component state. It also complains in the console on page load but I can live with that.

@fabriceci
Copy link

fabriceci commented Aug 11, 2017

I faced the same error, what a headache. I finally solved this, replacing the "stage-2" preset by "latest" in the babel.rc file. (Maybe someone can explain why this solve the problem !?)

Before (bug) :

"presets": [ "stage-2", "react" ]

After:

"presets": [ [ "latest", { "es2015": { "modules": false } } ], "react" ]

@noinkling
Copy link

noinkling commented Aug 12, 2017

@fabriceci Likely because "latest" (which is no longer recommended, in favor of the "env" preset) includes transform-es2015-classes, while "stage-2" doesn't. See a couple of the comments further up.

@fabriceci
Copy link

@noinkling thanks for you answer.

I changed my presets to :

"presets": [ "stage-2", "react", [ "env", { "targets": { "browsers": ["chrome > 55"] }, "loose": true, "modules": false } ] ]

I notice "Transform-es2015-classe" is included by the "env preset" only if it fits the browser's target config. With my config above, I'm forced to add the plugin manually. But, for exemple, if you target IE11, the plugin will be applied by the "env preset" automatically (so no need to add it manually).

transform-es2015-classes {"ie":"11"}`

It seems illogical to have to add the plugin if the env preset said you don't need it.

@ravenscar
Copy link

ravenscar commented Sep 6, 2017

I am not transpiling ES6 classes down to ES5 and am also having this problem using react-hot-loader.

I think the issue is with the react-hot-loader/patch not playing well with native ES6 classes when the bind() is done in the constructor.

If I examine the this value in the constructor() vs in componentWillMount() it is different. I expect the constructor has the original component and the componentWillMount() has the proxy created by react-hot-loader/patch. The proxy ends up being mounted by react.

When setState() is called in the bound function, it calls it on the original component, which is not mounted, rather than the proxy.

This definitely seems like an issue with the react-hot-loader/patch code, however most people are not noticing it as they are compiling ES6 classes down to ES5.

fixes:

  • Compile down to ES5
    • pro: works as it used to
    • con: using es5
  • Remove react-hot-loader/patch from webpack config.
    • pro: es6 hotloading of components will work
    • con: components not proxied, so state is lost
  • Don't bind in constructor
    • con: have to bind/use arrow functions at point of passing to other components (performance/antipattern)
    • con: may cause problems with event subscriptions that cause renders
  • Bind in componentWillMount() instead of constructor
    • con: non-standard so may cause other problems down the line.

I'm going to compile down to ES5 until react-hot-loader/patch works with native ES6 classes, as I think it's the safest option.

@HHogg
Copy link

HHogg commented Oct 3, 2017

Just another confirmation, we had this issue from binding methods in the constructor. With the same error and also loosing context for those components.

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op.

Using babel-plugin-transform-es2015-classes also fixed the issue.

@victorandree
Copy link

victorandree commented Oct 18, 2017

We have encountered the same errors as @HHogg on a TypeScript project (target ES2017 in dev, so it doesn't compile classes) with React 15.6.2 and react-hot-loader 3.1.1.

We don't bind methods in the constructor but in the class body,

class MyComponent extends React.Component<any, any> {
  constructor(props) {
    super(props);
    this.state = { message: 'Hello' };
  }

  public render() {
    return <button onClick={this.updateState}>{this.state.message}</button>;
  }

  private updateState = () => {
    this.setState({ message: 'New state' });
  }
}

Reverting to 3.0.0-beta.7 solves the issue for us.

@gregberge
Copy link
Collaborator

gregberge commented Dec 26, 2017

Related to #662 and Babel transpilation. Fixed in next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests