RHL is recreating class arrow functions which breaks event listeners

[gch1p]

Let's say I have a component like this:

class A extends Component {
    componentDidMount() {
        window.addEventListener('resize', this.onResize)
    }

    componentWillUnmount() {
        window.removeEventListener('resize', this.onResize)
    }

    onResize = () => {
        this.setState({...})
    }

    ...
}

This is my babel 7 config:

{
    presets: [
      [
        "@babel/preset-env",
        {
          loose: true,
          modules: "commonjs",
          targets: {
            chrome: "66",
          }
        }
      ],
      "@babel/preset-react"
    ],
    plugins: [
      'react-hot-loader/babel',
      'add-module-exports',
      ["@babel/plugin-proposal-decorators", {legacy: true}],
      ["@babel/plugin-proposal-class-properties", {loose: true}],
    ]
  }

Babel with plugin-proposal-class-properties transpiles above component to roughly something like this:

class A extends Component {
    constructor() {
        super()
        this.onResize = () => {
            this.setState({...})
        }
    }

    componentDidMount() {
        window.addEventListener('resize', this.onResize)
    }

    componentWillUnmount() {
        window.removeEventListener('resize', this.onResize)
    }
}

When hot reloading happens, this.onResize is redefined and points to a new function, not the function that was previously registered as a resize listener in didMount. When component gets unmounted, removeEventListener obviously doesn't work because this new onResize listener has never been registered, and as a result I have memory leak, React complaining about calling setState on an unmounted component and all sorts of bugs.

For now, I just hacked the RHL's code to prevent updating arrow functions at all (src/proxy/inject.js):

        if (!isArrow && (
          nextString !== String(prevAttr) ||
          (injectedBefore && nextString !== String(injectedBefore))
        ))

Perhaps you will come up with a more proper solution.

[theKashey]

Unfortunately, this is how RHL works. The only ways to solve the problem from our side

• do not update arrow functions (no, we have to)
• update classes as a while, calling componentWillUnmount and componentDidMount - that's not quite compatible with the way React is used today, and would break more that fix.

How to solve then?

• use a separate component to inject events, like react-event-injector. It would handle props(onResize) change

<TargetedInjector 
onResize={this.onResize}
target={window}
/> 

• use hooks, where mount is paired with unmount.

useEffect(() => {
  const onResize = () => setState(...);
   window.addEventListener('resize', onResize);
   return () =>  window.removeEventListener('resize', onResize);
}, []);

[gch1p]

Thank you for the answer. Perhaps it's worth adding it to the README.

I like the event injector helper solution, but since I already have quite a large project, it's not an option to rewrite everything just to fix RHL's side-effects.

For now I'm fine to stick to the non-updating-arrow-functions hack. It solves more than breaks. I could even add some ignoreArrowFunctions config option and send you a PR if you will accept it.

[theKashey]

Anyway - you have pointed on a very serious problem, which are able to fix.

In short - we should not modify this.something, but wrap that something with, well, something.

class A{
  onResize = () => {
        this.setState({...})
    }
}

// become 

class A{
  onResize = RHLWRAP('onResize', () => {
        this.setState({...})
   })
}

That would allow:

1. Replace internal implementation keeping top-level reference untouched
2. Regenerate all methods (right now we are updating only methods with this inside)

Only a simple babel plugin is needed.

The only problem - that would not work for .bind methods without special hacks (like overriding .bind).
Probably it would be easier to fully remount problematic classes, as react-refresh is going to do.

[theKashey]

class A{
  onResize = () => {
        this.setState({...})
    }
}

// become 

class A{
  onResize = () => RHLWRAP(() => {
        this.setState({...})
   })
}

// wraps realFunction with a "controllable" decorator
const RHLWRAP = (realFunction) => {
  let fn = realFunction;
  const result = (...args) => fn(...args);
  result.__getRealFunction = () => fn;
  result.__setRealFunction = (newFn) => fn=newFn;

  return result;
}

// onUpdate
// 1. Don'tt `this.method=eval(that.method.toString());
// 2. Do `this.method.__setRealFunction(eval(that.method.__getRealFunction.toString());

Again - this is not a solution. While that would update the function body - external dependencies, as well as any local variables would be old. As long as eval function would be executed in the "new" class instance - all the environment would be also new.

The proper way here - is to remove arrow function.

class A{
  onResize = () => RHLWRAP(function () {
        return this.setState({...})
   })
}

The method itself still bound to the instance, but there is no need to use eval to regenerate method body with this preserved.

// RHLWRAP should be plain function
function RHLWRAP(realFunction) {
  let fn = realFunction;
  const result = (...args) => fn.call(this,...args); // and call `fn` with `this`
  result.__getRealFunction = () => fn;
  result.__setRealFunction = (newFn) => fn=newFn;

  return result;
}

// 2. Do `this.method.__setRealFunction(that.method.__getRealFunction);

That would solve at least half of the problems we have today - ie #1024.
@gch1p - 👍🔥, I was just waiting for the right men to create the right issue. Спасибо большое.

[gch1p]

To be honest I'm surprised no one has reported this before. (Не за что.)

[theKashey]

I am also very surprised why nobody reported it at least a year ago.

[laino]

I believe a related problem would be this pattern:

public Whatever extends Component {
    public constructor(props) {
        super(props);

        this.myEventListener = this.myEventListener.bind(this);
    }
}

Now when you do .on('event', this.myEventListener) somewhere, then when react-hot-loader re-creates the element, this.myEventListener will have changed and .removeListener('event', this.myEventListener) won't work.

This way you can easily end up with accidentally registering many more event listeners than you intended...

Anyways. In theory it would be possible to detect this stuff and make it work properly - either by patching .bind(...) and making sure the same bound placeholder function is returned or by comparing instance properties after executing constructor (the latter would be pretty unsafe).

[theKashey]

This case is more or less easily fixable - during the update we detect bound methods, and rebound them. If we would run this operation just after component initialization - we might detect it and replace by a "constant ref", and updating it later. Almost the same approach as with arrow functions.

[laino]

Yup. We just patch .bind and attach .originalFunction and .boundArgs to the result, which would be transparent. Then it is later easy to compare this stuff. One could even just patch EventListener and dom node's addEventListener/removeEventListener to just perform the deduplication there.

In any case this stuff would be kind of "extra" and might break some setups. Maybe a small plugin system to opt-in into such functionality would be appropriate? Otherwise probably just another config toggle.

As in:

hotLoader.on('postCreate', (newComponent, proxyComponent) => {
    // Custom magic
});

hotLoader.on('postPatch', (oldComponent, newComponent, proxyComponent) => {
    // Custom magic
});

Then one could ship functionality that patches event listeners and .bind (which not everyone might want) like this:

 // register as a plugin with react-hot-loader as above and patches .bind etc.
import * as boundFunctionPatch from 'react-hot-bound-function-patch';

boundFunctionPatch.patchDOMEvents();
boundFunctionPatch.patchEventEmitter(EventEmitter);

// Many apps have multiple implementations of these floating around
boundFunctionPatch.patchEventEmitter(MyCustomEventEmitter);

boundFunctionPatch.config({
    replaceBoundInstanceFunctions: true // replace bound functions after constructor call
});

It would also make sense to ship a pre-patched EventEmitter with the package, so it can be aliased in one's webpack config to forego the ugly hacks.

Alternatively I think there's already a way to hook hot reload on a per-component level (I forgot what it is called though).

Anyways. I'd try to create a package like the above if there was a plugin system to support it. We'd be messing with a lot of internals though and I'm not sure how that might work out in every browser.

[theKashey]

Patching .bind is not a good idea. It requires patching something we should never touch. Patching addEventListener is an interesting option - you may do it for window, but not for every DOM node.

The problem with arrow and bound functions exists as long as RHL updates them. If it will not - the problem would disappear. So - this is the only moment we should change.
Right now RHL compares class methods and prototype - if it sees the same "name" here and there - it will rebound it, causing the problem.

So - RHL should run "check" after class creating, "understand" what it would update in the future, and "fix" it creating a temporal function, which it will never update, which will call the "real" one.