Support MobX (HOC Decorators)

[alexilyaev]

Description

Two things I've noticed:

1. When using @inject from mobx-react, changing something in render causes the component to re-mount, instead of hot reloaded
2. If there's an exception in componentWillUnmount in that case, the browser will reload and we don't see the exception in the console, unless we enable Preserve Log (which is off usually).

Expected behavior

• Ideally components wrapped with an HOC should hot-reload instead of re-mount
• Exceptions during the the unmount lifecycle should not cause full browser reload

Actual behavior

The component re-mounted and the browser reloaded.

Environment

"react-hot-loader": "3.0.0-beta.7",
Node 6.9.1.
npm 5.0.3
macOS 10.12.5
Chrome 59

Reproducible Demo

Tried to dumb it down to a minimal reproduction demo:
https://github.com/alexilyaev/react-hmr-uncaught-error

• Once it's up, change "Dynamo 123" under app/components/navigator/navigator.js to something else and save.
• At this point the browser reloads and you shouldn't see any hint in the browser as of why that happened.
• Enabling Preserve Log in DevTools should show the errors:
  [HMR] Cannot apply update. Need to do a full reload!
  and then the 123 error.

Thanks.

[mhaagens]

I don't get the [HMR] Cannot apply update. Need to do a full reload! error, but I had the same problem and once I removed inject from MobX the component stopped remounting on change.
Wish there was a way to use inject with HMR though.

[mhaagens]

Same thing happens with withRouter from React Router. Remount instead of replacing.

[theKashey]

This is expected. Patch cant see all components of a class

@inject('todos')
@observer
class Navigator extends React.Component ...

Will give you ONE class. But by the fact, you are creating TREE!
First (exported, extracted to a variable) one will be registered in RHL, but internal classed - will be not. RHL will be unable to change them using react-proxy.

See https://codeburst.io/how-to-hot-load-react-component-in-7-days-part-2-react-28ce2b61d0c7
or #608
It is just very hard, and sometimes impossible, to use HoC or decorators. And there is no solution, yet.

[cristianfraser]

Is there any update on this?

[alexilyaev]

Still an issue.
I've upgraded to latest versions of React, react-hot-loader and MobX.

• Still causes a remount when changing something in render
• Exceptions in componentWillUnmount still cause a page reload
• Also, if I remove @inject and leave only @observer, the HMR doesn't work at all (meaning, changing something in render does not hot-reload the component or anything)

I do get something new if I comment out the throw I have in componentWillUnmount and change something in render:

patch.dev.js:138 React Hot Loader: this component is not accepted by Hot Loader. 
Please check is it extracted as a top level class, a function or a variable. 
Click below to reveal the source location: 
 ƒ Navigator() {
    __WEBPACK_IMPORTED_MODULE_1_babel_runtime_helpers_classCallCheck___default()(this, Navigator);

    var _this = __WEBPACK_IMPORTED_MODULE_3_babel_runtime_helpers_possibleConstructor…

The Navigator component looks fine though:

import React from 'react';
import { inject, observer } from 'mobx-react';

@inject('todos')
@observer
class Navigator extends React.Component {
  constructor() {
    super();

    console.log('Navigator: constructor');
  }

  componentWillUnmount() {
    console.log('Navigator: componentWillUnmount');
    // throw new Error('123');
  }

  render() {
    return (
      <div>
        <h1>
          Dynamo 1234566786
        </h1>
      </div>
    );
  }
}

export default Navigator;

If I ditch the decorators and use them directly:

• export default inject('todos')(Navigator); - Works as expected, without the patch.dev.js:138 error
• export default inject('todos')(observer(Navigator)); - Does not hot-reload at all

[gregberge]

We have several known issues that explain this result:

• decorators are not supported Support decorators #279
• only exposed components can be reloaded HotLoader won't work after adding >=2 levels of HoC. #650

There are complicated to solve, help is welcome.

[gregberge]

@theKashey this is linked to #279 right? We should focus on this case and implement a MobX example.

[theKashey]

Was failing with v3 - cos MobX hides class from RHL, and it will be unmounter
Was failing with v4 - cos you dont have any AppContainer or hot.
Also your way to module.accept is just wrong.

If you will wrap Routes with hot - everything will be ok

import React from 'react';
import {hot} from 'react-hot-loader';
import { BrowserRouter, Route, Switch } from 'react-router-dom';

import Root from '../root/root';

export default hot(module)(function Routes() {
  return (
    <BrowserRouter>
      <Switch>
        <Route exact path="/" component={Root} />
      </Switch>
    </BrowserRouter>
  );
})

[alexilyaev]

@theKashey I've updated the repo with v4.0.0-beta.6 and updated the missing parts based on the instructions in getting-started:

• Added <AppContainer>
• Proper use of module.hot.accept

I've also tried your example wrapping hot around Routes (with and w/o).

The test repo link again:
https://github.com/alexilyaev/react-hmr-uncaught-error

Results

After changing 123 in navigator.js > render:

• I don't get the warning anymore (yay!)
• But, the component is re-mounted (or something), twice! I see:

Root render
Navigator: constructor
Hello: constructor
+ Root render
+ Navigator: constructor
+ Navigator: constructor
+ Root render

• componentWillUnmount was not called at all, how can that be if the constructor was called?
• Wrapping Routes with hot(module)
  • Suppressed componentWillUnmount call somehow
  • Doesn't help with the re-mounting

Continued...

• Removing the decorators above Navigator (leaving plain class), even weirder:

Root render
Navigator: constructor
Hello: constructor
+ Navigator: constructor
+ Navigator: constructor
+ Navigator: constructor
+ Navigator: constructor
+ Root render
+ Root render

• When not wrapping Routes with hot(module), componentWillUnmount is called and the throw is shown in the console as expected (does not reload the page as it originally did 👍)

[theKashey]

Oh hot module update RHL will “probe” all components, creating and rendering them.
So this output is ok.

[alexilyaev]

Ahm, for a second I got confused as what was I expecting from RHL in the first place.

So I watched Dan Abramov's talk on Hot Reloading with Time Travel again.

The key points, in my words:

• RHL proxies the methods of the component, so when you change something in render, it just calls the new render
• The component does not re-mount, does not lose it's state

Wasn't that the whole point of RHL? (besides not reloading the whole page)

As it is now, the component does remount, and I would lose component state.

[gregberge]

@alexilyaev I fork your project add I added a demo, no mount/unmount occurs. The constructor is called but it is the normal behaviour.

[alexilyaev]

@neoziro Oh, I stand corrected, constructor and render do run again but without a remount, I'll some more tests.

But this is true only with the hot(module) wrapping around Routes, without it it does remount.

Will post some more test results soon.

[theKashey]

@alexilyaev - you have to wrap your Application with AppContainer or use hot(module) which, actually, do the same.
Without any of them you will got remount.

Also - Dan's video was about RHL v1. Here we got v4 - it is a bit different.
Keys change - to handle arrow functions (they catch this) we cannot use react-proxy.
The new approach has to understand the change, and it literally creates an OldComponent and a NewComponent on hot-change, to probe and understand the difference.
Next it copies over that changes back to the old component.

All messages you can saw - is from dry-run. They are not related to the real application.

[alexilyaev]

Good News

I've updated the repo to have running counters:

• Hello - Has a React state counter (by setInterval in componentDidMount)
• Navigator - Has a MobX @observable counter (by setInterval in componentDidMount)

Results

• With hot(module) around Routes, everything works as expected, changes in any of the components does not re-mount anything and both counters continue running.
• Without it, Hello and Navigator gets re-mounted, the React state is reset (obviously) and MobX state doesn't (since it's in a separate module).
• Interesting finds:
  • Removing <AppContainer> and the module.hot.accept in Root.js, everything still works 😮
  • Removing react-hot-loader/patch from webpack.config.js > entry, still works

Conclusions

• As far as I'm concerned, this issue is fixed and the latest RHL supports MobX decorators just fine.
• Is <AppContainer> and module.hot.accept in Root.js still needed? react-hot-loader/patch?
• Wrapping Routes with hot(module) is not documented I think, and without it it doesn't work

[theKashey]

AppContainer and hot do the same. hot wraps Component you provided with AppContainer and setup webpack module.hot.accept. Thus means - do not hot-accept App.js. Wrap with hot only nested Components (Root/Routes)
You need at least one of AppContainer/hot. Lots of hot just could help in some situations, as long they are able to scope the change.
And it is documented in v4(next) branch.

[gregberge]

The next documentation, it is very simple now.

[alexilyaev]

Oh cool, what a relief 😊.

Closing.
Thanks for the support.