Fix: observer should not modify original componentClass

[zhangciwu]

When use as function, observer modifies original componentClass
It could cause Maximum call stack size exceeded exception after componentClass's render method got too many wrapping

It happens to me, and I create a repo to reproduce it: https://github.com/zhangciwu/mobx-react-stack-exceeded

This PR is to fix it by extending (clone) componentClass before modifying, and keeping original class out of altering.
Also appended some test cases.

[mweststrate]

@zhangciwu thanks for this PR. Few questions

1. Does this somehow impact react hot loading? You could tests the impact of that after rebasing on master and checking this repo
2. Although this setup is nicer (so i'll might still merge this MR if RHL doesn't see new problems), the real problem in your example / test is that you are generating components on the fly during render. Please avoid doing this as this will kill React's reconciliation mechanism; every time you render the same thing, you render it with a completely new and different (as far as React knows) component, so React now has to render the entire subtree of components, as it never saw this type of component before.

So instead of writing:

        function getWraped() {
            return inject(stores => ({
                name: stores.userStore.name
            }))(observer(NameDisplayer))
        }

You could simply write:

const WrappedNameDisplayer =  inject(stores => ({
                name: stores.userStore.name
            }))(observer(NameDisplayer))
        }

There is no need to define a new component here every time you are rendering the display name

[zhangciwu]

For Q1, tested with mobx-react-boilerplate, it's all OK when I edited components render or store functions.

For Q2, the below code is like react pure function component, and can be more dynamic, like adding some dynamic options when generates this component. And I use this function for (nearly) root node, should be fine cause when store changes, root would not be re-rendered, only children nodes can be re-rendered

[mweststrate]

Wondering, will targetClassName now be the reported name in exceptions and React devtools and such?

[ematipico]

It will, so it's worth implementing .displayName

[zhangciwu]

Yeah, just added displayName for targetClass

[mweststrate]

Is the right branch ever taken?

[mweststrate]

Ok, thanks for the clarification! Left some more questions. Might be nice to split the code flow a bit to make explicitly clear which code is taken for a component class / function component / class react.createClass component, etc?

[zhangciwu]

Actually there is code to tell if it's Stateless function component at line 342 and below, and handling this branch after judging.

And after Line 369, componentClass should be component class / react.createClass, cause other circumstances has been handled above and returned.

For component class / react.createClass, just extend should be fine, cause extend is actually dealing with prototype of parent class or class-like-function.
This extend just put componentClass.prototype into targetClass.prototype.prototype (to prevent componentClass.prototype being modified), should works fine for both component class / react.createClass

[mweststrate]

Ok, thanks! Will test this extensively and if everything works out fine make part of the next release Op ma 20 aug. 2018 om 12:47 schreef zhangciwu <notifications@github.com>:

…

Actually there is code to tell if it's Stateless function component at line 342 and above, and handling the branch after judging. For component class / class react.createClass, just extend should be fine, cause extend is actually dealing with prototype of parent class or class-like-function. This extend just put componentClass.prototype into targetClass.prototype.prototype (to prevent componentClass.prototype being modified), should works fine for both component class / class react.createClass — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#523 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhCjIb5ULN1RykH7juxMgS0Ne2CLtks5uSpO4gaJpZM4VlFPS> .

[Strate]

This will introduce huge BC break, because of missing statics and methods on new component. As for now, observer do not produce a new class, so it keep all statics and methods on component instance.

[mweststrate]

@Strate I think that should keep working as is because the new component inherits from the old one? Probably we should make sure we have some unit tests that make sure we don't have regressions here

[zhangciwu]

Inheritance wouldn't drop class methods, including static ones, just seek one more step on prototype chain. (So it's called inheritance)
Unit tests are welcome to test corner cases, @Strate if you have some code which is broken after this, just post it here.

[Strate]

@zhangciwu @mweststrate oh, sorry, I've missed thing that newely created class extends original.

[mweststrate]

No problem! I'm happy you critically reviewed the code :) So feel free to leave any concerns! Op di 18 sep. 2018 om 08:56 schreef Artur Eshenbrener < notifications@github.com>:

…

@zhangciwu <https://github.com/zhangciwu> @mweststrate <https://github.com/mweststrate> oh, sorry, I've missed thing that newely created class extends original. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#523 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhFm1uUIMjqH2iJhg34mdGqGRisdZks5ucJkUgaJpZM4VlFPS> .

[mweststrate]

When trying to use the changes locally, I get the following error on any class component:

Uncaught TypeError: Class constructor Router cannot be invoked without 'new'
    at new targetClassName (:8001/static/builds/7.15.1/main.js:1183)
    at constructClassInstance (:8001/static/builds/7.15.1/vendor.js:221916)
    at updateClassComponent (:8001/static/builds/7.15.1/vendor.js:223400)
    at beginWork (:8001/static/builds/7.15.1/vendor.js:223786)
    at performUnitOfWork (:8001/static/builds/7.15.1/vendor.js:225785)
    at workLoop (:8001/static/builds/7.15.1/vendor.js:225849)
    at HTMLUnknownElement.callCallback (:8001/static/builds/7.15.1/vendor.js:216103)
    at Object.invokeGuardedCallbackDev (:8001/static/builds/7.15.1/vendor.js:216142)
    at invokeGuardedCallback (:8001/static/builds/7.15.1/vendor.js:215999)
    at renderRoot (:8001/static/builds/7.15.1/vendor.js:225927)

[mweststrate]

Happens when use ES6 targeting typescript, any idea @zhangciwu? Upgrading to this package to babel7 (see #554) doesn't seem to fix the problem

[zhangciwu]

@mweststrate
form stack info I can get something

• You have a class called Router
• Router got wrapped by observer (got a class meaning you use src instead of compiled file)
• wrapped Router called without new, so error happens

So, do you use src instead of compiled index.js or index.module.js ? compiled files have no class keyword
And providing more code context would help, including observer usage part and Router source

[mweststrate]

Hey @zhangciwu, sorry, I probably won't be able to create a small repro until Monday, but here are the most interesting details:

1. We use typescript, targetting es6, so class keywords should be preserved

Router is declared as:

@stateObserver
export class Router extends React.Component<IModelerViewProps> {

where stateObserver itself is declared as

export function stateObserver<P extends IReactComponent>(clazz: P): P {
    return inject("modelerState")(observer(clazz as any));
}

Compiled down to:

let Router = class Router extends React.Component {
    render() {
        return "stuff"
    }
};
Router = __decorate([
    essentials_1.stateObserver
], Router);
exports.Router = Router;

[zhangciwu]

How do you import observer?
If you import from local mobx-react package, it would use module and typings entries in packages.json, the module entry is compiled file, would not preserve class keyword; typings entry is type declaration, just type definations.

If you import from '..somepath/mobx-react/src', class keyword would preserve, but this is not recommanded, try modified to the way above.

[mweststrate]

class from mobx-react is not preserved, (using import from "mobx-react"), it is the class of Router that is being preserved and used here (since, targetting ES6)

[zhangciwu]

Seems Router's usage or rendering procedure may cause this, but not very clear.
What if use older version?

at new targetClassName (:8001/static/builds/7.15.1/main.js:1183)
Are this line and several lines around (of compiled main.js) convinient to post?

[mweststrate]

sure

  var targetClass =
  /*#__PURE__*/
  function (_componentClass) {
    _inherits(targetClassName, _componentClass);

    function targetClassName() {
      _classCallCheck(this, targetClassName);

      return _possibleConstructorReturn(this, _getPrototypeOf(targetClassName).apply(this, arguments));
    }

    return targetClassName;
  }(componentClass);

  targetClass.displayName = componentClass.displayName || componentClass.name;
  var target = targetClass.prototype || targetClass;
  mixinLifecycleEvents(target);
  targetClass.isMobXReactObserver = true;
  makeObservableProp(target, "props");
  makeObservableProp(target, "state");
  var baseRender = target.render;

Edit: this is the babel 7 compiled version of mobx-react, as it wat supposed to fix several class check issues

[zhangciwu]

https://stackoverflow.com/questions/36577683/babel-error-class-constructor-foo-cannot-be-invoked-without-new

This shall be the root cause
you cannot extend a native class with a transpiled class.

Thus, you still need to compile source code to es5 or compile mobxreact into es6

[mweststrate]

Both are not options imho:

• we cannot force all mobx-react consumers to transpile down their dependencies from es6 to es5
• we cannot force all mobx-react consumers to not target es6

[zhangciwu]

If need to publish es2015 (preserve class)
This might help

https://babeljs.io/blog/2018/06/26/on-consuming-and-publishing-es2015+-packages

A common suggestion is for libraries to start publishing ES2015 under another field like es2015, e.g. "es2015": "es2015/package.mjs".

// @angular/core package.json
{
  "main": "./bundles/core.umd.js",
  "module": "./fesm5/core.js",
  "es2015": "./fesm2015/core.js",
  "esm5": "./esm5/core.js",
  "esm2015": "./esm2015/core.js",
  "fesm5": "./fesm5/core.js",
  "fesm2015": "./fesm2015/core.js",
}

So much targets make npm community ecology fragile, hope there will be a good solution for this

[zhangciwu]

As I noticed, mobx 5.x is ES6 only, 4.x is ES5 compatible
Maybe open a new major version to preserve class in this major version, and maintain two major versions like mobx?

[sergei-startsev]

Here's a related issue in babel: babel/babel#9367.