⁉[DISCUSSION]: Should the state from storage override or merge with an updated default state

[DanielHabenicht]

Affected Package

The issue is caused by package @ngxs/storage-plugin

Is this a regression?

No (idk)

Description

The default value is not present if a new property is added to a state that was persisted to local storage before.

If a property is not in local storage it should fall back to the default values defined in the state.

🔬 Minimal Reproduction

1. Create a State with a property, e.g.:

@State<{
  property1: string;
}>({
  name: 'myState',
  defaults: {
    property1: 'default'
  }
})

2. Use it in your app in conjunction with @ngxs/storage-plugin and persist the state in the localstorage.

3. Add another property to the state:

@State<{
  property1: string;
  newProperty: string;
}>({
  name: 'myState',
  defaults: {
    property1: 'default',
    newProperty: 'this should be default'
  }
})

4. Read the the state.
   ⚠️ The new property is not there / on read it returns undefined.

Reproduction:
https://stackblitz.com/edit/ngxs-empty-action-handler-bcvdty?file=src%2Fapp%2Fapp.component.ts

Please note that I set the local storage to what would be there after step 2 in main.ts.

Environment

Libs:
- @angular/core version: 9.1.3
- @ngxs/store version: 3.6.2
- @ngxs/storage-plugin version: 3.6.2


Browser:
- [x] Chrome (desktop) version Version 81.0.4044.113 (Official Build) (64-bit)
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: v10.15.3
- Platform:  Windows

Others:

[markwhitfeld]

I have experienced this exact same thing, but just nuked my storage to fix the issue.
The same issue is present if you update a value in a default state... would you expect it to override storage?

I'm not sure that there is an easy answer to this, so I made this a DISCUSSION issue.

Currently, storage state just overrides the default state.
Is this expected? Is this a reasonable default?
The other 'automated' option would be to merge it with the default state and potentially also remove values from the stored value if they are the same as the default.

I think that we could provide a mechanism for letting the developer choose how the storage gets merged with the default state.... maybe an addition to the new beforeSerialise and afterDeserialise hooks? ... or something else?

Also, I'm not sure about where HMR fits in with this? @splincode?
@davidmorissette I would like to get your thoughts on this too.
cc @splincode

[DanielHabenicht]

I don't think it should override the storage. I think it should more be like a fallback.

Like:

let storageState = {
  property1: "bla"
}

let defaultState = {
  property1: "default",
  newProperty: "default"
}

let new State = {
  ...defaultState, 
  ...storageState
}
// Outcome
// {
      property1: "bla",
      newProperty: "default"
// }

It should not remove Properties, as this would potentially break something.
I am open to letting the developer decide if it should behave that way. Although I think this is expected behaviour.

[splincode]

afterDeserialise - should help with this, since it will be possible to combine states

[DanielHabenicht]

Thanks, I will have a look at it. Maybe I can add my implementation to this library than :)

[davidmorissette]

1. Create the state using the default values
2. Overwrite the value of the properties contained in the storage

So similar of what @DanielHabenicht described I guess. Thanks to the new 'afterDeserialize' option, it is now possible to change this behavior if needed.

[davidmorissette]

I just had an extra tought about this. It depends if we use the "deeply nested via dot notation" or not. If we do, then logically I would do what I describe in my previous message. Otherwise, I would use the whole state contained in the storage and ignore the default values.

So for me there is a difference between the behaviors depending on the intention:

• I am using the dot notation because I want to be pin point and store only certain properties
• I am storing the whole state so I want it back the way it was when I stored it

[DanielHabenicht]

Some issues I have run into:

• the deserialize option only offers obj as input instead of obj and key as stated here: https://github.com/ngxs/store/blob/master/docs/plugins/storage.md#serialization
• How can I get the default values of a State by the states name? (I want to implement the default behaviour for all states without being explicit)

[markwhitfeld]

@DanielHabenicht Just to note that the afterDeserialise we are talking about is only available on the latest dev release, and not the stable release.

[DanielHabenicht]

Ohhh, ok. I will take a look at Dev then.

The documentation and implementation is different anyway. Which one is right? (obj or obj and key)

[ilyakonrad]

We ended up creating an object which must be updated once a part of a cached store is changed structurally.

We keep this object in localStorage as well. Then on application initialization we check whether local version of this object is different from the cached in localStorage one, and if so - we update localStorage object while logging user out and resetting the state 🙈

There's page reload happening when user logs in (kind of technical debt) and thus the store gets initialized from scratch with default values (due to localStorage is reset).

const storeVersion = {
  version: 6,
  latestUpdate: 'Update sidebar styles'
}

In app.component.ts constructor:

const previousStoreVersion = JSON.parse(localStorage.getItem('storeVersion'))
if (
  !previousStoreVersion ||
  previousStoreVersion.version !== storeVersion.version ||
  previousStoreVersion.latestUpdate !== storeVersion.latestUpdate
) {
  this.store.dispatch([new StateReset(...PERSISTED_STATES), new Logout()]).subscribe(() => {
    localStorage.setItem('storeVersion', JSON.stringify(storeVersion))
  })
}