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

Invalid snapshot when model prop changed in onAttachedToRootStore #74

Closed
sisp opened this issue Dec 19, 2019 · 2 comments · Fixed by #75
Closed

Invalid snapshot when model prop changed in onAttachedToRootStore #74

sisp opened this issue Dec 19, 2019 · 2 comments · Fixed by #75
Labels
🐛 bug Something isn't working 👨‍💻 has PR A PR that potentially fixes the issue exists 🎈 released A fix that should close the issue has been released

Comments

@sisp
Copy link
Contributor

sisp commented Dec 19, 2019

I think a test case that reproduces this problem explains it best:

import {
  getSnapshot,
  Model,
  model,
  modelAction,
  prop,
  registerRootStore,
} from 'mobx-keystone';

test('snapshot', () => {
  @model('#74/A')
  class A extends Model({
    value: prop<number>(),
  }) {
    public onAttachedToRootStore() {
      this.value = 1; // but, e.g, this works: setTimeout(() => { this.value = 1; })
    }
  }

  @model('#74/Store')
  class Store extends Model({
    all: prop<A[]>(() => []),
  }) {
    @modelAction
    public add(a: A): void {
      this.all.push(a);
    }
  }

  const store = registerRootStore(new Store({}));
  store.add(new A({ value: 0 }));

  expect(getSnapshot(store).all).toHaveLength(store.all.length);
});

The state is correct, but the snapshot is wrong. Unfortunately, I haven't been able to determine the cause of this error.

@xaviergonz xaviergonz added the 🐛 bug Something isn't working label Dec 19, 2019
@xaviergonz xaviergonz mentioned this issue Dec 19, 2019
@xaviergonz xaviergonz added the 👨‍💻 has PR A PR that potentially fixes the issue exists label Dec 19, 2019
@xaviergonz
Copy link
Owner

xaviergonz commented Dec 19, 2019

Thanks! should be fixed in 0.28.3, please reopen if not.
The fix adds a slight change in behaviour though. onAttachedToRootStore and its disposer will now run after the current action / runUnprotected block is finsihed (it will still run synchronously though).

xaviergonz added a commit that referenced this issue Dec 19, 2019
* Fix: modifying a node inside onAttachedToRootStore or its returned disposer no longer results in broken snapshots

* update dev deps
@xaviergonz xaviergonz added the 🎈 released A fix that should close the issue has been released label Dec 19, 2019
@sisp
Copy link
Contributor Author

sisp commented Dec 20, 2019

Works perfectly, thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 👨‍💻 has PR A PR that potentially fixes the issue exists 🎈 released A fix that should close the issue has been released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants