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

Abstract class property and ExtendedModel in CRA app causes error: "Error: data changes must be performed inside model actions" #18

Closed sisp closed 5 years ago

sisp commented 5 years ago

I'm afraid #17 didn't solve everything.

abstract class A extends Model({}) {
  // Comment out the line below and it works fine!
  public abstract value: number;
}

@model("B")
class B extends ExtendedModel(A, { value: prop<number>() }) {}

// Error: data changes must be performed inside model actions
new B({ value: 1 }); // Instantiating the class inside `runUnprotected` works.

Running the above snippet inside a CRA app results in the following error:

Error: data changes must be performed inside model actions

When running the snippet with ts-node directly, there is no problem.

sisp commented 5 years ago

And this might be a related issue:

abstract class A extends Model({}) {
  // Comment out the line below and it works fine!
  public abstract value: number;
}

@model("B")
class B extends ExtendedModel(A, { value: prop<number>() }) {}

// `runUnprotected` is currently needed here as described above.
const b = runUnprotected(() => new B({ value: 1 }));
console.log(b.value); // is `undefined` in a CRA app, but `1` when run with `ts-node`
xaviergonz commented 5 years ago

The issue seems to be that the abstract property seems to be actually setting value to undefined after it has been initialized to 1 by the base constructor...

sisp commented 5 years ago

But why is it only happening in a CRA app (Babel + Typescript) and not with ts-node?

xaviergonz commented 5 years ago

Most probably typescript is doing the right thing (not initializing it at all) but babel is instead setting it to undefined on its constructor (wild guess without checking the generated js code)

sisp commented 5 years ago

This is the code generated with CRA:

class A extends Object(mobx_keystone__WEBPACK_IMPORTED_MODULE_0__["Model"])({}) {
  constructor(...args) {
    super(...args);
    this.value = void 0;
  }

}

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, {
  value: Object(mobx_keystone__WEBPACK_IMPORTED_MODULE_0__["prop"])()
}) {}) || _class); // `runUnprotected` is currently needed here as described above.

const b = Object(mobx_keystone__WEBPACK_IMPORTED_MODULE_0__["runUnprotected"])(() => new B({
  value: 1
}));
console.log(b.value); // is `undefined` in a CRA app, but `1` when run with `ts-node`
xaviergonz commented 5 years ago

Care to test that branch to see if it fixes it? basically it will ignore any property changes made on constructors and enable changes to be made once all constructors have been run

sisp commented 5 years ago

Works for the example above. :+1:

xaviergonz commented 5 years ago

That being said I think it should be reported to the babel guys, I don't think they are supposed to generate any setting code for abstract props