Throws when key is not in storage

[rafamel]

Hi there, awesome work!

There's an issue, however, when the storage key doesn't exist. Of course, it can be catched, and then after the first save happens, it won't throw again for that key, but it might be sensible to check for the existence of a key in storage and, if it doesn't exist, save the initial value, if it exists, only then retrieve and hydrate.

Otherwise we'll get ( w/ localforage):

Error: [mobx-state-tree] Error while converting `null` to `AnonymousModel`:
value `null` is not assignable to type: `AnonymousModel` (Value is not a plain object).

[agilgur5]

Thanks for reporting this! I noticed this for the first time last week too when I cleared my cache. The workaround is of course to set the key to an empty object {} if it doesn't exist EDIT: workaround is to set to initial state, see below comments, but I'll have to add that default here too soon.
I think I tested to make sure JSON.parse and JSON.stringify don't throw, but not that so MST doesn't throw and so missed this with a cleared cache.

[rafamel]

Hi @agilgur5 , sounds like a plan! Yes, setting the key to an empty object would work as long as the store is not initialized with some arbitrary state, in which case it would be re-hydrated with an empty object instead of keeping the original state. Of course the key could be set with that initial state instead of an empty object but seems redundant and appears to defeat the purpose of abstracting these details. I think the library following the above strategy would be a good default, but I'll leave that to your judgement :)

Thanks for the quick response, Anton!

[agilgur5]

Oh that's actually a good point, I didn't think about non-empty initial state.

I'm not sure if applySnapshot merges the snapshot with the current state or just sets it to the new one. I think it might be the latter but will have to check.

Yea the solution here would then be not to set it to an empty object, but to just skip the applySnapshot call iff the storage returns null

[agilgur5]

Ok, seems like the Bookshop MST example also skips applySnapshot when falsey.

And this test shows that it falls back to the default of the model instead of the initial state when an empty object is applied. The source code (took a bit of digging) for the model type's applySnapshot is here and looks like it just sets each key to the snapshot's key (with defaults to the model defaults).

So yea, your point on initial state applies (nice catch!), and skipping the call would be best practice and a safe bet per the example.

And yea the workaround right now would just be to set the storage key to the initial state if it doesn't exist until its implemented here.

[agilgur5]

I've actually never used create() with any arguments before (i.e. no initial state besides defaults), so that use case also points to some potential gotchas when having an initial state with blacklisted attributes -- the persisted state will just throw away the blacklisted initial attributes since applySnapshot does not do a merge. However, with this fix, blacklisted initial attributes before anything is persisted will still exist. Not sure if that's intended behavior or not 🤔

[agilgur5]

Besides the considerations in the comment above and some missing tests, PR is out on #2. Can use the fix with npm i -S agilgur5/mst-persist#fix-nonexistent-key until its merged and published (or do the workaround in user-code).

[rafamel]

Hi @agilgur5, sorry for the delay on this and thank you for taking the time to take a look on this so promptly!

I am not sure on what you mean by "blacklisted initial attributes before anything is persisted will still exist". As far as I can see it, that would always be the case. Meaning, if I'm hydrating the store with some initial values, they should of course still exist, they just shouldn't be hydrated from permanent storage on future reloads -that is, they should always initialize to their initial create or default state. I think I'm failing to see the issue with this, could you please further clarify?

If what you are saying is that when using applySnapshot blacklisted properties will be set to null/undefined or they would overwrite the create value whenever we're hydrating the store from persistent storage, I would say this would be faulty behavior, that is, when hydrating the store blacklisted keys should keep their create initial state (if any). If that were the issue you were referring to, I would suggest:

• If no persisted state exists, do nothing (current (fix): handle non-existent key in storage #2 implementation)
• If persisted state exists, retrieve current in-memory store state, then shallow merge (as no nested key can be white/blacklisted) with whitelisted persisted state.

[agilgur5]

I think you understood correctly @rafamel . The problem with #2 is that no merging is ever performed in mst-persist; applySnapshot is a set, not a merge. So #2 creates some potentially unexpected behavior when comparing these two situations:

1. No persistence state exists, initial state has a blacklisted attribute -- it still exists after persistence.
2. App is quit immediately after creation of initial state. App is re-opened, blacklisted attribute no longer exists.

These two states are very similar, but have different (potentially unintended) results.

MST doesn't have a method to merge snapshots as far as I can tell, so it seems like this library might have to eventually implement something like redux-persist's State Reconcilers/merge strategies.

I think I'll merge #2 for now as it resolves a critical bug, but will have to open another issue to resolve state merge strategies later.

[rafamel]

In that case I think what I was pointing out applies then? Assuming blacklisted/whitelisted keys are only for the store root object (so not nested), the logic for reconciling state should be fairly simple. Unless there's something I'm missing?

 return storage.getItem(name)
    .then((data) => {
      const snapshot = !jsonify ? data : JSON.parse(data)
      applySnapshot(
        store,
        blacklist || whitelist ? Object.assign(store.toJSON(), snapshot) : snapshot
      )
    })

[agilgur5]

Yes, a shallow merge like you mentioned is an option.

Whether or not it's simple isn't what I was referring to however -- a shallow merge is not necessarily the behavior a user wants or expects. MST doesn't define "merging" either, so we can't just delegate to MST for that behavior. redux-persist allows one to choose merge strategies and defaults to a shallow merge -- ideally, that behavior should be replicated for mst-persist for good API parity and DX.

I think #2, "'do nothing' if there is no persisted state" is the most intuitive option, but it may cause some gotchas without any merge strategies (it's critical enough that I think it should be pushed without them however). Shallow merge is the most intuitive default, but merge strategies should ideally be configurable and explicitly mentioned in the docs. Merge strategies are a separate issue, but are related in a few ways.
EDIT: sorry for the edits, I was overthinking the "no persisted state exists" case, do nothing is definitely most intuitive.

[agilgur5]

Fixed and released in v0.0.2. Let's move any further discussion on merge strategies to #5

[agilgur5]

Hi @lucas-wilmart . Sorry, but what you're experiencing is different, I've added 100% test coverage since this issue was made, and it covers the case when no key is in storage multiple times.

as soon as I tried to add a key in the white-list (in my case 'accounts') in order to persist a single store, mobx-state-tree throwed an error about the tree not being compatible (very similar to OP's)

Similar errors, but very different causes. This issue is an old bug around when nothing was in storage (an edge case as usually there's something in storage), not about whitelists etc.
Your addition of a whitelist means a property doesn't exist while your types say it should. What you're seeing is a duplicate of #25 and that's a duplicate of #5, which is originally brought up here (literally the comment right above yours mentions it). #5 is a feature request, not a bug.

And To be honest, I didn't fully understand everything in this thread.

Yea that's to be expected. This is the first issue and we covered a lot of ground here about related issues. The original title, description, and PR that fix it all still stand. What you're experiencing is related, hence the mention of #5 here, but not the same.

I would probably suggest filing a separate issue if you're not sure/confused about something. Easier to close duplicates than redirect existing conversations.

I copy/pasted the persist function declaration in this repo and figured it out that it was because the snapshot applyied to my store didn't had the "login" key. So I added my root store initial state as an argument of the function to merge it with the snapshot object created from the local storage in order to apply it to the tree. That way, my initial state for my non-persisted keys is set.

Yep that is the topic of #5 . Please up-vote that if that is what you'd like.

Here's my own modifed version of the persist function to comply with my needs if it can help anyone: [...] (I removed the code which handles different storage system since I don't need anything else than regular localStorage, if you use this code and need other storage system you'll have to rewrite it sorry about that)

Sorry, I'm inclined to mark this as off-topic as, other than being a different issue, putting a wholesale copy of a big portion of the codebase with only two lines added and many removed is bound to be even more confusing to any readers -- I'm the author and it was hard for even me to tell what changed.
A patch or PR would be easier to understand for readers as it would be a diff and could eventually make its way into the codebase. A PR for #5 is certainly welcome, especially as that is gaining some popularity.

[lucas-wilmart]

@agilgur5 Thanks a lot for your very detailed answer. I understand why you marked my answer as off-topic, sorry about that. I thought it could help people facing the same situation I did.