zoomsphere / ngx-store

Angular decorators to automagically keep variables in HTML5 LocalStorage, SessionStorage, cookies; injectable services for managing and listening to data changes and a bit more.
https://www.npmjs.com/package/ngx-store
ISC License
167 stars 41 forks source link

Array call such as push right after initialization of LocalStorage member #33

Closed HarelM closed 6 years ago

HarelM commented 6 years ago

Hi,

I can't fully explain this but I'm seeing an issue when using this library on an array in incognito mode (I saw it on chrome). The following is the screen shot of my debug session: As can be seen I'm trying to push a value to an array and for some reason the array is null (or the proxy to it). image Might have been solved in later versions, I'm using 1.2.3

The reproduction is fairly simple and can be done simply by surfing in incognito mode to the following address: https://israelhiking.osm.org.il/debug/#!/?s=C2ul3KFWTZ This is the original caller: image

Original code that uses this library can be found here: https://github.com/IsraelHikingMap/Site/blob/master/IsraelHiking.Web/sources/application/services/layers/layers.service.ts

Let me know if there's anything I can do to help.

HarelM commented 6 years ago

After further debug I think the root cause of this issue is the fact that string array (string[]) does not create a local storage key. After deleting all the local storage keys and running the site I get the following list: image The following key is missing: ngx_activeOverlayKeys Although it is defined here: https://github.com/IsraelHikingMap/Site/blob/ccffff30891f1b255a8964fd0ff40ab9c8b33ebf/IsraelHiking.Web/sources/application/services/layers/layers.service.ts#L54

I'll try to update to latest version and see if it helps later tonight.

DanielKucal commented 6 years ago

@HarelM, please let me know is it still an issue in v1.3.5 and RC of v1.4 (npm i ngx-store@RC), thanks in advance

HarelM commented 6 years ago

The same issue exists with 1.3.5 and with 1.4.0RC1 :-(

HarelM commented 6 years ago

Anything I can do to help out? I really need this fixed. I can probably use the service to workaround this issue as when the key is created it works, but I prefer to have a fix here...

HarelM commented 6 years ago

Ok, I looked on issue #19 to see if I can debug the issue better and I saw that this issue was opened on the same key which might indicate it's the same issue... Do you have a test that covers the issue #19 to make sure this is not that again?

HarelM commented 6 years ago

I'm sure I don't fully understand all the proxy cache item etc but I did encounter something that look odd: The following line declares a variable which is the same as the calling function: https://github.com/zoomsphere/ngx-store/blob/ed68b94a5d1721e6bda30d5952d5af5e8a4c43dd/src/decorator/cache-item.ts#L84 I'll see if changing that variable help in anyway, but I doubt it...

HarelM commented 6 years ago

Nope, that wasn't it but I think I managed to find something interesting: So the call goes like this:

  1. getProxy which returns an empty array: []
  2. push which goes to the _self.readValue(config);
  3. readValue sets var value = null;
  4. iterating through utilities gives nothing and thus it get to the copy implemented by json.Parse and Json.stringify
  5. but now getProxy is called with null and not undefined so it won't break in the undefined line: if (value === undefined && this.proxy) and will set proxy to null and return null which fails the whole sequence. Which is related to this commit which is fairly recent: https://github.com/zoomsphere/ngx-store/commit/e0cdff43ee507caec2c8ae94e97e9a2f8c60e48f

I'm sure I'm not helping much, I can upload a site that is not minified so you can debug, it might prove to be better...?

HarelM commented 6 years ago

Sorry for spamming... Ok, I think I managed to narrow down the issue and understand why this issue happens on one of my arrays while it doesn't happen on the others: The first interaction I have with this array is the push method - this is the first one after initialization. So when calling cacheitem: https://github.com/zoomsphere/ngx-store/blob/ed68b94a5d1721e6bda30d5952d5af5e8a4c43dd/src/decorator/cache-item.ts#L84 it will call readValue: https://github.com/zoomsphere/ngx-store/blob/ed68b94a5d1721e6bda30d5952d5af5e8a4c43dd/src/decorator/cache-item.ts#L97 Which will return null since this value was only initialized and never set. I have managed to solve it by saving all the values when initializing like so:

    public saveValue(value: any, config: DecoratorConfig = {}): any {
        debug.groupCollapsed('CacheItem#saveValue for ' + this.key + ' in ' + this.currentTarget.constructor.name);
        debug.log('new value: ', value);
        debug.log('previous value: ', this.readValue(config));
        debug.log('targets.length: ', this.targets.length);
        debug.log('currentTarget:', this.currentTarget);
        debug.groupEnd();

        // prevent overwriting value by initializators
        if (!this.initializedTargets.has(this.currentTarget)) {
            this.initializedTargets.add(this.currentTarget);
            let readValue = this.readValue(config);
            let savedValue = (readValue !== null) ? readValue : value;
            let proxy = this.getProxy(savedValue, config);
            proxy = (proxy !== null) ? proxy : value;
            debug.log('initial value for ' + this.key + ' in ' + this.currentTarget.constructor.name, proxy);
            this.utilities.forEach(utility => utility.set(this._key, savedValue, config));
            this.saveValue(savedValue, config); // NOTE: this is the lines that fixes this.
            return proxy;
        }
        this.utilities.forEach(utility => utility.set(this._key, value, config));
        return this.getProxy(value, config);
}

There might be a better solution but this does the job and also makes sure all values are stored while initializing which can prevent mismatch between in memory and localStorage on initialization.

Let me know what you think, I can do a pull request if needed.

HarelM commented 6 years ago

Last one, I promise. I've looked at the RC, kudos on adding the tests! Please make sure to add tests for arrays, as I believe this issue can make an excellent test case. Keep up the good work!

DanielKucal commented 6 years ago

Hey @HarelM, I've just added test cases for array variable like you described here: https://github.com/zoomsphere/ngx-store/pull/32/commits/5cd4ecfb73a78481228c5c81219fdb460a4688fe And it seems to work... Give v1.4.0RC4 a try and show me test case that would fail, please.

HarelM commented 6 years ago

Thanks!! seems like RC4 does the trick :-) I'm guessing that the following commit is what solves this issue: https://github.com/zoomsphere/ngx-store/commit/9cabf4d06175499efaaccd9a55ea6d04b4ab694f

DanielKucal commented 6 years ago

Good to hear that the problem is gone!

HarelM commented 6 years ago

Let me know when it is release officially (no longer RC).

DanielKucal commented 6 years ago

You can safely use RC for now, it has some tests coverage while version <1.4 do not and you shouldn't spot any breaking changes (maybe if you use more than 1 decorator on 1 variable you will need to change its order). I will let you know after release ;-)