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

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

Closed
sisp opened this issue Aug 30, 2019 · 8 comments · Fixed by #17
Closed

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

sisp opened this issue Aug 30, 2019 · 8 comments · Fixed by #17

Comments

@sisp
Copy link
Contributor

sisp commented Aug 30, 2019

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
Copy link
Contributor Author

sisp commented Aug 30, 2019

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
Copy link
Contributor Author

sisp commented Aug 30, 2019

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:

const base: any = baseModel || BaseModel
class CustomBaseModel extends base {}

@xaviergonz
Copy link
Owner

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
Copy link
Owner

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

@xaviergonz
Copy link
Owner

Basically

  • FC (function class) extends FC is ok
  • class extends class is ok
  • class extends FC is ok
  • FC extends class is broken

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
Copy link
Contributor Author

sisp commented Aug 31, 2019

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
Copy link
Owner

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
Copy link
Contributor Author

sisp commented Aug 31, 2019

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

Indeed, thanks a lot, again! :-)

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

Successfully merging a pull request may close this issue.

2 participants