search

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

applySnapshot messes with onAttachedToRootStore #103

Closed martinheidegger closed 4 years ago

martinheidegger commented 4 years ago

This might be related to #27.

/ Short

Last night I tried to figure out why I got following execution order:

.onAttachedToRootStore .onAttachedToRootStore (detachMethod)

and it seems that applySnapshot will mess with the onAttachedToRootStore hooks. I worked around it by only counting how many calls to either are made and only execute onAttached... the first time and only execute detach once its called at an equal amount as the onAttached.. amount. But it would be nice if it would be executed in order.

/ Long

In my code I have a User Model that is restored/persisted in the .onAttachedToRootStore hook. When the app is started, the user is created and attached to the rootStore. Once all data is loaded .applySnapshot is called to restore the user. There is also a Vault child of User that will be restored with the user. There is also a VaultData model that is to be restored/replicated from a different physical location when attached to the root store. The VaultData is conceptually linked Vault as Vault.dataKeyHex contains the storage location of the VaultData. So: once there is a new Vault, I need to also create/restore the VaultData with it. In a previous version I tried using onInit to restore the data. (more about the current version later). It worked somehow but I noticed that the VaultData.onAttachedToRootStore method was called more than once, screwing with my restore logic (the store was restored twice). I attempted to make sure that in it can be destructed at any time and still I ran into weird states.

... so i tried to debug mobx-keystone...

I figured out that both onAttachedToRootStore calls are triggered by this block:

https://github.com/xaviergonz/mobx-keystone/blob/93103c520c35fa1a97eba4fa256305d664db9a75/packages/lib/src/parent/setParent.ts#L112-L119

where the listener execution is added to a queue, which is filled twice for the same object before being executed with applySnapshot. I noticed that when executing on... here

https://github.com/xaviergonz/mobx-keystone/blob/93103c520c35fa1a97eba4fa256305d664db9a75/packages/lib/src/rootStore/attachDetach.ts#L23-L25

before the execution onAttachedDisposers.get(ch) was already defined, causing in an overwrite (and loss) of the previous disposer without it ever being called.

I didn't find a good way for a test yet but I worked around this by storing the VaultData in a separate rootStore, counting how often .onAttachedToRootStore is called on the Vault and only running it the first time while registering the destruction to be called once the counter is back to 0.

Now this workaround somehow does the job, even if it feels fishy, but it would be good if .onAttachedToRootStore was called only once, or alternatively if the destruct method to be called before the next onAttachedToRootStore call.

xaviergonz commented 4 years ago

Seems like a bug, is there any chance you could provide a unit test / code that exposes the problem though? That'd make it much easier to fix.

xaviergonz commented 4 years ago

By the way, are both calls to onAttachedToRootStore made with the same rootStore as parameter or different ones?

xaviergonz commented 4 years ago

I just released v0.35.0 with some changes related to this, let me know if it fixes anything

martinheidegger commented 4 years ago

It seems to be fixed, but let me test for a little more.

xaviergonz commented 4 years ago

Nice, is it fixed because now the hook only triggers once or because they trigger twice but the disposer does get called properly?

xaviergonz commented 4 years ago

@martinheidegger can the issue be closed then?