From 2f9542d9f44d6b18d463ebcda5decf3c5b97ae18 Mon Sep 17 00:00:00 2001 From: Ryan Hutchison Date: Mon, 21 Jun 2021 09:21:52 -0400 Subject: [PATCH] fix(storage-plugin): only restore state if key matches addedStates (#1746) * fix(storage-plugin): only restore state if key matches addedStates * test(storage-plugin): add test that ensures that state is not restored again * chore: update CHANGELOG.md Co-authored-by: arturovt --- CHANGELOG.md | 1 + bundlesize.config.json | 4 +- .../bundlesize.config.json | 2 +- .../bundlesize.config.json | 2 +- packages/storage-plugin/src/storage.plugin.ts | 15 ++- ...-restore-state-only-if-key-matches.spec.ts | 119 ++++++++++++++++++ 6 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 packages/storage-plugin/tests/issues/issue-restore-state-only-if-key-matches.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ea796d28c..1dd3f5045 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ $ npm install @ngxs/store@dev - Fix: Devtools Plugin - Do not connect to devtools when the plugin is disabled [#1761](https://github.com/ngxs/store/pull/1761) - Fix: Router Plugin - Cleanup subscriptions when the root view is destroyed [#1754](https://github.com/ngxs/store/pull/1754) - Fix: WebSocket Plugin - Cleanup subscriptions and close the connection when the root view is destroyed [#1755](https://github.com/ngxs/store/pull/1755) +- Fix: Storage Plugin - Only restore state if key matches `addedStates` [#1746](https://github.com/ngxs/store/pull/1746) - Performance: Tree-shake errors and warnings [#1732](https://github.com/ngxs/store/pull/1732) - Performance: Tree-shake `ConfigValidator`, `HostEnvironment` and `isAngularInTestMode` [#1741](https://github.com/ngxs/store/pull/1741) - Performance: Tree-shake `SelectFactory` [#1744](https://github.com/ngxs/store/pull/1744) diff --git a/bundlesize.config.json b/bundlesize.config.json index da7cad8db..7a53fbfb8 100644 --- a/bundlesize.config.json +++ b/bundlesize.config.json @@ -116,14 +116,14 @@ "path": "./@ngxs/storage-plugin/fesm2015/ngxs-storage-plugin.js", "package": "@ngxs/storage-plugin", "target": "es2015", - "maxSize": "12.000KB", + "maxSize": "12.65KB", "compression": "none" }, { "path": "./@ngxs/storage-plugin/fesm5/ngxs-storage-plugin.js", "package": "@ngxs/storage-plugin", "target": "es5", - "maxSize": "13.630KB", + "maxSize": "14.29KB", "compression": "none" }, { diff --git a/integrations/hello-world-ng11-ivy/bundlesize.config.json b/integrations/hello-world-ng11-ivy/bundlesize.config.json index 845c4d33c..094206240 100644 --- a/integrations/hello-world-ng11-ivy/bundlesize.config.json +++ b/integrations/hello-world-ng11-ivy/bundlesize.config.json @@ -3,7 +3,7 @@ { "path": "./dist-integration/main.*.js", "target": "es2015", - "maxSize": "251.00 kB", + "maxSize": "251.05 kB", "compression": "none" } ] diff --git a/integrations/hello-world-ng12-ivy/bundlesize.config.json b/integrations/hello-world-ng12-ivy/bundlesize.config.json index 372856b25..37a17a89f 100644 --- a/integrations/hello-world-ng12-ivy/bundlesize.config.json +++ b/integrations/hello-world-ng12-ivy/bundlesize.config.json @@ -3,7 +3,7 @@ { "path": "./dist-integration/main.*.js", "target": "es2015", - "maxSize": "236.60 kB", + "maxSize": "236.65 kB", "compression": "none" } ] diff --git a/packages/storage-plugin/src/storage.plugin.ts b/packages/storage-plugin/src/storage.plugin.ts index 518db9132..8e7cf46fe 100644 --- a/packages/storage-plugin/src/storage.plugin.ts +++ b/packages/storage-plugin/src/storage.plugin.ts @@ -42,11 +42,20 @@ export class NgxsStoragePlugin implements NgxsPlugin { // transformed by the `storageOptionsFactory` function that provided token const keys = this._options.key as string[]; const matches = actionMatcher(event); - const isInitAction = matches(InitState) || matches(UpdateState); + const isInitAction = matches(InitState); + const isUpdateAction = matches(UpdateState); + const isInitOrUpdateAction = isInitAction || isUpdateAction; let hasMigration = false; - if (isInitAction) { + if (isInitOrUpdateAction) { for (const key of keys) { + // We're checking what states have been added by NGXS and if any of these states should be handled by + // the storage plugin. For instance, we only want to deserialize the `auth` state, NGXS has added + // the `user` state, the storage plugin will be rerun and will do redundant deserialization. + if (isUpdateAction && event.addedStates && !event.addedStates.hasOwnProperty(key)) { + continue; + } + const isMaster = key === DEFAULT_STATE_KEY; let val: any = this._engine.getItem(key!); @@ -89,7 +98,7 @@ export class NgxsStoragePlugin implements NgxsPlugin { return next(state, event).pipe( tap(nextState => { - if (!isInitAction || (isInitAction && hasMigration)) { + if (!isInitOrUpdateAction || (isInitOrUpdateAction && hasMigration)) { for (const key of keys) { let val = nextState; diff --git a/packages/storage-plugin/tests/issues/issue-restore-state-only-if-key-matches.spec.ts b/packages/storage-plugin/tests/issues/issue-restore-state-only-if-key-matches.spec.ts new file mode 100644 index 000000000..f12de17c5 --- /dev/null +++ b/packages/storage-plugin/tests/issues/issue-restore-state-only-if-key-matches.spec.ts @@ -0,0 +1,119 @@ +import { APP_BASE_HREF } from '@angular/common'; +import { Router, RouterModule } from '@angular/router'; +import { BrowserModule } from '@angular/platform-browser'; +import { Component, NgModule, NgZone } from '@angular/core'; +import { platformBrowserDynamic } from '@angular/platform-browser-dynamic'; +import { freshPlatform } from '@ngxs/store/internals/testing'; +import { State, NgxsModule, Store, StateToken } from '@ngxs/store'; + +import { NgxsStoragePluginModule } from '../../'; + +describe('Restore state only if key matches', () => { + beforeEach(() => { + // Caretaker note: it somehow sets `/@angular-cli-builders` as a default URL, thus when running `initialNavigation()` + // it errors that there's no route definition for the `/@angular-cli-builders`. + spyOn(Router.prototype, 'initialNavigation').and.returnValue(undefined); + }); + + afterEach(() => { + localStorage.clear(); + }); + + it( + 'should not deserialize the state twice if the state should not be handled by the storage plugin', + freshPlatform(async () => { + // Arrange + interface AuthStateModel { + token: string; + } + + const AUTH_STATE_TOKEN = new StateToken('auth'); + + localStorage.setItem( + AUTH_STATE_TOKEN.getName(), + JSON.stringify({ + token: 'initialTokenFromLocalStorage' + }) + ); + + @State({ + name: AUTH_STATE_TOKEN, + defaults: null + }) + class AuthState {} + + @State({ + name: 'user', + defaults: {} + }) + class UserState {} + + @Component({ template: '' }) + class UserComponent {} + + @NgModule({ + imports: [ + RouterModule.forChild([ + { + path: '', + component: UserComponent + } + ]), + NgxsModule.forFeature([UserState]) + ], + declarations: [UserComponent] + }) + class UserModule {} + + @Component({ selector: 'app-root', template: '' }) + class TestComponent {} + + @NgModule({ + imports: [ + BrowserModule, + RouterModule.forRoot([ + { + path: 'user', + loadChildren: () => UserModule + } + ]), + NgxsModule.forRoot([AuthState]), + NgxsStoragePluginModule.forRoot({ + key: [AuthState] + }) + ], + declarations: [TestComponent], + bootstrap: [TestComponent], + providers: [{ provide: APP_BASE_HREF, useValue: '/' }] + }) + class TestModule {} + + // Act + const { injector } = await platformBrowserDynamic().bootstrapModule(TestModule); + const ngZone = injector.get(NgZone); + const router = injector.get(Router); + const store = injector.get(Store); + const subscribeSpy = jest.fn(); + const subscription = store.select(AuthState).subscribe(subscribeSpy); + + // Well, we're explicitly setting another value for the auth state before going to the `/user` route. + // Previously, it would've retrieved the auth state value from the local storage each time the `UpdateState` + // action is dispatched. See `storage.plugin.ts` and `!event.addedStates.hasOwnProperty(key)` expression which + // runs `continue` if the state, which has been added, shouldn't be handled by the storage plugin. + localStorage.setItem( + AUTH_STATE_TOKEN.getName(), + JSON.stringify({ token: 'manuallySetTokenToEnsureItIsNotRetrievedAgain' }) + ); + + await ngZone.run(() => router.navigateByUrl('/user')); + + // Assert + expect(subscribeSpy).toHaveBeenCalledTimes(1); + expect(subscribeSpy).toHaveBeenCalledWith({ + token: 'initialTokenFromLocalStorage' + }); + + subscription.unsubscribe(); + }) + ); +});