Store: Breaking change in storage plugin in v3.3.0

[mflorence99]

I'm submitting a...

[x] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => https://github.com/ngxs/store/blob/master/CONTRIBUTING.md
[ ] Other... Please describe:

Current behavior

The fix for issue #538 changed line 26 of storage.plugin.ts to:

if (typeof val !== 'undefined' && val !== null) {

However, on line 61, getValue for a key that does not yet exist will return undefined. In turn, setItem on line 65 will serialize that as the string'undefined':

if (key !== '@@STATE') {
  val = getValue(nextState, key);
}

try {
  this._engine.setItem(key, options.serialize(val));

The effect is that the test on line 26 passes, the deserialization of the string 'undefined' fails and the value is set to an empty object.

Expected behavior

Instead, the value should ultimately be set to its default state, as it was in v3.2.0.

I think the test on line 26 could be coded as:

if (val !== 'undefined' && val != null) {

I have patched it to be so in node_modules/@ngxs/storage-plugin/fesm5/ngxs-storage-plugin.js and everything behaves as expected.

Note that val != null is equivalent to val !== undefined && val !== null as per https://stackoverflow.com/questions/38648087/checking-for-null-or-undefined.

[splincode]

PR please

[mflorence99]

Sure, if you agree I'm on the right track, I'll do that right away!

[eranshmil]

The reason for that PR was to fix custom engine under NativeScript.
If you revert that, you will break for other users.
It should be thoroughly investigated.

[splincode]

I think you need to add a test for current case

[mflorence99]

Sure thing - I had time this morning for a PR for my suggested change but not to dive into the test cases. I’ll do that ASAP after the holiday. Also - my apologies - I do not have the context to properly grok the NativeScript implications.

[eranshmil]

Maybe the PR author could help. @MrCroft

[MrCroft]

Hi everyone,
Sorry for such a late answer, saw this in the morning but couldn't take time to have a look during the work day. I'm home now.
Let me pull current master branch and see what's happening exactly. I know the issue is to the point, maybe too "to the point" for me 😀
Also, I'm not sure I understand the desired (or appropriate) solution?

What's strange is that I did test with the build I've done back then, and the application worked.

Note that val != null is equivalent to val !== undefined && val !== null .

@mflorence99 I actually didn't know that... what started this was that when working with applicationSettings (mobile development), the first time I would start the app (fresh/clean build) I would get this: Error occurred while deserializing the store value, falling back to empty object. and the state wasn't initialized with the defaults I provided in my state class under defaults: { ... }.
Then, looking at the getItem implementation:

getItem(key: any) {
  return applicationSettings.getString(key);
}

I've checked what does applicationSettings.getString(key) return for a key that doesn't exist, and it returns undefined (as opposed to null when using localStorage in the browser). With that in mind, I actually thought that the check for 'undefined' without typeof ~was a mistake~ behaved weirdly somehow. Not sure what made me look that way at the time, but after doing a build with the change applied, the application worked as expected.

Anyway, let me have a look now.

---

EDIT: on the other hand, while still using 3.2.0, I could easily make it work by replacing the getItem implementation: return applicationSettings.getString(key); with return applicationSettings.getString(key) || null;. I guess what I'm trying to say is that a "revert" of that PR wouldn't be a disaster (3.3.0 just being released, unless you're like me have an OCD to check daily and quickly update everything) and would be very easy to manage by the developer "user of @ngxs", since the custom storage implementation is up to the developer anyway.

[eranshmil]

@MrCroft Thanks for your reply.
A quick patch for all cases would be changing line 61 to:

  val = getValue(nextState, key) || undefined;

Obviously, it should be tested using all the available engines.

@markwhitfeld What do you think about this solution?

Edit:
On second thought, it would be depended on the implementation of setItem.

[mflorence99]

@eranshmil My apologies for interjecting, but my tests showed that getValue for a key that does not yet exist already returns undefined. The code on line 65 will serialize that as the string 'undefined' which will pass the test on 26.

So in effect that patch would be:

val = /* getValue(nextState, key) -> */ undefined || undefined;

[eranshmil]

You're right, I misunderstood the issue.

There are a few changes to try to cover all possible return values from the storage engines.
But I think the best approach is to add to the storage plugin documentation what value should be set for an undefined key.

[MrCroft]

@mflorence99 hmm... seems legit.
And as long as undefined is serialized as 'undefined', then it shouldn't have passed the original if (val !== 'undefined' && val !== null) {... check.
However, I've just double-checked and:

1. my getItem() implementation (return applicationSettings.getString(key);) does return undefined
2. right before line 26 (if (typeof val !== 'undefined' && val !== null) {) val is undefined instead of 'undefined'

Does that mean that there's something going on with the serialization part? And that it was never discovered, since most people are likely to use localStorage (which returns null) for custom engines as well (although I'm not that experienced to know why would one write a custom implementation for any other reason except to replace the default storage). But I would guess that this kind of thing (val getting to be undefined instead of 'undefined' before line 26) would be caught by tests.

---

EDIT:
I've completely missed the serialization part, if I hadn't - maybe I would have realized that the strange thing is that undefined doesn't become 'undefined', and wouldn't have gone through with my PR in that case.

[markwhitfeld]

@mflorence99 and @MrCroft I created and merged a PR with the following fix in line 26 to handle both scenarios:
if (val !== 'undefined' && typeof val !== 'undefined' && val !== null) {

The @dev npm package should contain this fix, please could you both confirm that this is fixed for you?
Once confirmed I will publish a release.

[MrCroft]

@markwhitfeld yup, just checked to make sure. Working fine on my end!
Thank you!

[mflorence99]

Yes -- works perfectly! Many thanks @markwhitfeld and @MrCroft, @eranshmil and @splincode for helping out so quickly.

[splincode]

Will be 3.3.1

[markwhitfeld]

3.3.1 was released today with this fix. Closing issue.