sisp commented 5 years ago

After upgrading to v0.17, I'm getting the following error:

mobx-keystone.esm.js:110 Uncaught Error: assertion failed: value is not ready to take a parent
    at failure (mobx-keystone.esm.js:187)
    at mobx-keystone.esm.js:2017
    at executeAction (mobx.module.js:713)
    at res (mobx.module.js:701)
    at tryUntweak (mobx-keystone.esm.js:3251)
    at Array.interceptObjectMutation (mobx-keystone.esm.js:3428)
    at interceptChange (mobx.module.js:3202)
    at ObservableObjectAdministration.write (mobx.module.js:4393)
    at _set (mobx.module.js:2833)
    at Object.set (mobx.module.js:3156)
    at BaseModel.set (mobx-keystone.esm.js:5166)
    at BaseModel.setProps (p.ts:29)
    at executeAction (mobx.module.js:713)
    at BaseModel.res (mobx.module.js:701)
    at BaseModel.wrappedAction (mobx-keystone.esm.js:1750)
    at BaseModel.update (o.ts:121)
    at update (n.ts:72)
    at BaseModel.onAttachedToRootStore (n.ts:89)
    at executeAction (mobx.module.js:713)
    at BaseModel.res (mobx.module.js:701)
    at BaseModel.wrappedAction (mobx-keystone.esm.js:1750)
    at mobx-keystone.esm.js:1894
    at walkTreeParentFirst (mobx-keystone.esm.js:1838)
    at walkTree (mobx-keystone.esm.js:1831)
    at attachToRootStore (mobx-keystone.esm.js:1891)
    at mobx-keystone.esm.js:2083
    at executeAction (mobx.module.js:713)
    at res (mobx.module.js:701)
    at internalTweak (mobx-keystone.esm.js:3184)
    at executeAction (mobx.module.js:713)
    at res (mobx.module.js:701)
    at interceptArrayMutation (mobx-keystone.esm.js:3095)
    at interceptChange (mobx.module.js:3202)
    at ObservableArrayAdministration.spliceWithArray (mobx.module.js:3390)
    at Proxy.push (mobx.module.js:3544)
    at BaseModel.add (g.ts:91)
    at executeAction (mobx.module.js:713)
    at BaseModel.res (mobx.module.js:701)
    at BaseModel.wrappedAction (mobx-keystone.esm.js:1750)

There's no problem with v0.16. I suspect the cause of this problem has been introduced in #24. I'm calling an action of a child model instance in some parent model's onAttachedToRootStore which replaces the current (empty) array of SomeModel instances (prop<SomeModel[]>()) with a new array of SomeModel instances. I haven't had time to come up with a minimal example for reproduction. If you can't guess what might be causing this problem, I'll try to create a test case for you.

xaviergonz commented 5 years ago

If you could that'd be great, but I suspect it was introduced there too

sisp commented 5 years ago

This is the simplest case I could find that causes the error:

import { Model, model, modelAction, prop, registerRootStore } from "mobx-keystone"
// When the below line is uncommented, no error occurs.
// import { Model, model, modelAction, prop, registerRootStore } from "../src"

test("issue #27", () => {
  @model("#27/ModelWithArrayProp")
  class ModelWithArrayProp extends Model({
    values: prop<number[]>(),
  }) {
    onAttachedToRootStore(): void {
      this.setValues([1, 2, 3])
    }

    @modelAction
    public setValues(values: number[]): void {
      this.values = values
    }
  }

  const m = registerRootStore(new ModelWithArrayProp({ values: [] }))
  expect(m.values).toEqual([1, 2, 3])
})

Apparently, it's important that the values prop type is not primitive (e.g. an array). I've tested this case with v0.16 and it is passing there, so something has happened since then.

xaviergonz commented 5 years ago

Hmm, I just tried the test case and it passed

xaviergonz commented 5 years ago

are you using mobx4 or 5?

sisp commented 5 years ago

MobX 5. Hmm, let me try it again.

sisp commented 5 years ago

Uh, you're right. I'm not sure what happened. I'll investigate.

xaviergonz commented 5 years ago

if you want can you try https://github.com/xaviergonz/mobx-keystone/pull/28 (though it's a wild guess)

sisp commented 5 years ago

Okay, I can at least reproduce the error again. I accidentally imported the built version of mobx-keystone (from packages/lib/dist) instead of the source directly (e.g. ../src). The error does not occur when using the latter.

Steps to reproduce:

Clone the repository.
yarn install
yarn run lib:build
Create a file with the test case from above (with import { ... } from 'mobx-keystone', so the built version from dist will be used) somewhere in packages/lib/test.
cd packages/lib && yarn test

sisp commented 5 years ago

28 does not seem to fix the error.

sisp commented 5 years ago

Are you able to reproduce it now?

xaviergonz commented 5 years ago

Yep! really weird that it only happens with the dist version, will check it, thanks!

sisp commented 5 years ago

Yes, I'm surprised, too. Thanks a lot for looking into it!

xaviergonz commented 5 years ago

Heh, it was a bad babel transpilation of iterators. It didn't happen in tests since those use TS to compile, but it did happen with dist since that's compiled with babel

sisp commented 5 years ago

That's nasty ... I can confirm that #29 fixes the issue.

xaviergonz commented 5 years ago

Fix released as 0.17.2

xaviergonz / mobx-keystone

Error: assertion failed: value is not ready to take a parent #27

28 does not seem to fix the error.