xaviergonz / mobx-keystone

A MobX powered state management solution based on data trees with first class support for Typescript, support for snapshots, patches and much more
https://mobx-keystone.js.org
MIT License
549 stars 25 forks source link

create-react-app: TypeError: Class constructor [...] cannot be invoked without 'new' #15

Closed sisp closed 5 years ago

sisp commented 5 years ago

When using ExtendedModel in a create-react-app-based app, I'm getting the following error:

TypeError: Class constructor [...] cannot be invoked without 'new'

This is a minimal reproduction: https://github.com/sisp/mobx-keystone-cra-issue/tree/issue-15

The error does not occur when I run the example with ts-node, so I think the problem originates from CRA's usage of Babel + Typescript build chain.

sisp commented 5 years ago

I think there is a relationship with this TSDX PR. But two commits later, the Typescript config override was set back to before the PR. I think target: es5 wouldn't work for mobx-keystone anyway because ES6 sets are used, so it couldn't be transpiled to ES5 anyway.

sisp commented 5 years ago

I'm getting closer to understanding the problem. Here is a quote from @gaeron about this problem:

I’m not sure if you read the linked issue, but it’s not extending native classes that’s broken per se, but extending them using Babel transform. If A is native, but B is written with Babel class transform, and B extends A, it will break. This is a known Babel issue (I linked to it), and Babel likely won’t fix it because of performance concerns with the fully compatible solution. This doesn’t come up very often because it works the other way around. So it’s only a problem if a library uses Babel and that library happens to extend your class which might be native.

I think class CustomBaseModel extends base {} doesn't work when base is a class passed to ExtendedModel:

https://github.com/xaviergonz/mobx-keystone/blob/1e8e3db887451a936b4b81186ea9853867c44ef6/packages/lib/src/model/Model.ts#L160-L161

xaviergonz commented 5 years ago

It depends actually on the target of the user side transpiled code and the library transpiled code Basically, typescript, when transpiling to ES5 will create classes as functions, with a call to BASE.call(this, ...) to invoke the constructor, but with ES6 class you are forced to use new

There are two ways to fix it without touching the code 1) make the user side code transpile to ES5, so the ES5 library can call apply on the base class 2) create a es6 version of the library that keeps the non transpiled class

Basically, the user side and library must match (either es5 or es6)

Does CRA use the ESM version or the common js one? if so maybe the es version could be defaulted to compile to es6

xaviergonz commented 5 years ago

Or maybe it is just the time to ship ES6 by default, but that might keep people with old browsers out :(

xaviergonz commented 5 years ago

Basically

If the user code is ES6 and the library is ES5 then for a non extended model it will be

class -> FC (Model) -> FC (BaseModel)

but for extended model it is

class -> FC (ExtendedModel) -> class -> FC (Model) -> FC (BaseModel)

A solution keeping es5 is possible btw, but it requires proxies :-/

sisp commented 5 years ago

Does CRA use the ESM version or the common js one?

With #14 merged, CRA uses the ESM version. Before #14, it was using CommonJS.

if so maybe the es version could be defaulted to compile to es6

Does the following line from TSDX's Rollup config mean that TSDX compiles the library to ES5 because target: undefined when opts.target === 'browser'?

targets: opts.target === 'node' ? { node: '8' } : undefined,

There are two ways to fix it without touching the code

  1. make the user side code transpile to ES5, so the ES5 library can call apply on the base class

In the example I provided to reproduce this error, I believe Typescript is configured to transpile to ES5, isn't it?

"target": "es5",

Yet I'm getting the error.

  1. create a es6 version of the library that keeps the non transpiled class

What would need to be changed for this? By the way, since TSDX v0.9.0, TSDX's Rollup config can be customized.

Basically, the user side and library must match (either es5 or es6) ... A solution keeping es5 is possible btw, but it requires proxies :-/

Is my understanding correct that there is no way to support client code for both ES5 and ES6 simultaneously without ES6 Proxy? You're using ES6 Set (e.g. here), so doesn't this require the client target to be ES6 anyway? Or is ES6 Set polyfilled?

If there is no way to support ES5 and ES6 client code with the same library, would it make sense to build 2 versions, one for ES5 and one for ES6, and publish them separately? This would be somewhat similar to lodash and lodash-es I think.

xaviergonz commented 5 years ago

In the example I provided to reproduce this error, I believe Typescript is configured to transpile to ES5, isn't it? "target": "es5", Yet I'm getting the error.

Because CRA ignores that for development mode, if you look at the main.js file generated in dev mode in the chrome inspector you will see this

class A extends Object(mobx_keystone__WEBPACK_IMPORTED_MODULE_0__["Model"])({}) {} // Uncomment the below block and it works fine.
//
// @model("B")
// class B extends A {}

let B = (_dec = Object(mobx_keystone__WEBPACK_IMPORTED_MODULE_0__["model"])("B"), _dec(_class = class B extends Object(mobx_keystone__WEBPACK_IMPORTED_MODULE_0__["ExtendedModel"])(A, {}) {}) || _class); // TypeError: Class constructor A cannot be invoked without 'new'

Either way I think I found a solution with the PR that works for everything :)

sisp commented 5 years ago

Either way I think I found a solution with the PR that works for everything :)

Indeed, thanks a lot, again! :-)