perf(store): run change detection once for all selectors when asynchr…

…onous action has been completed
ngxs · May 21, 2022 · ffbb75f · ffbb75f
1 parent 28d6804
commit ffbb75f
Show file tree

Hide file tree

Showing 4 changed files with 118 additions and 53 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,7 @@ $ npm install @ngxs/store@dev
 - 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)
 - Performance: Tree-shake `deepFreeze` [#1819](https://github.com/ngxs/store/pull/1819)
+- Performance: Run change detection once for all selectors when asynchronous action has been completed [#1828](https://github.com/ngxs/store/pull/1828)
 - Performance: Router Plugin - Tree-shake `isAngularInTestMode()` [#1738](https://github.com/ngxs/store/pull/1738)
 - Performance: Tree-shake `isAngularInTestMode()` [#1739](https://github.com/ngxs/store/pull/1739)
 - Performance: Storage Plugin - Tree-shake `console.*` calls and expand error messages [#1727](https://github.com/ngxs/store/pull/1727)

diff --git a/packages/store/src/store.ts b/packages/store/src/store.ts
@@ -1,7 +1,14 @@
 // tslint:disable:unified-signatures
 import { Inject, Injectable, Optional, Type } from '@angular/core';
 import { Observable, of, Subscription, throwError } from 'rxjs';
-import { catchError, distinctUntilChanged, map, take } from 'rxjs/operators';
+import {
+  catchError,
+  distinctUntilChanged,
+  map,
+  publishReplay,
+  refCount,
+  take
+} from 'rxjs/operators';
 import { INITIAL_STATE_TOKEN, PlainObject } from '@ngxs/store/internals';
 
 import { InternalNgxsExecutionStrategy } from './execution/internal-ngxs-execution-strategy';
@@ -15,6 +22,17 @@ import { StateFactory } from './internal/state-factory';
 
 @Injectable()
 export class Store {
+  /**
+   * This is a derived state stream that leaves NGXS execution strategy to emit state changes within the Angular zone,
+   * because state is being changed actually within the `<root>` zone, see `InternalDispatcher#dispatchSingle`.
+   * All selects would use this stream, and it would call leave only once for any state change across all active selectors.
+   */
+  private _selectableStateStream = this._stateStream.pipe(
+    leaveNgxs(this._internalExecutionStrategy),
+    publishReplay(1),
+    refCount()
+  );
+
   constructor(
     private _stateStream: StateStream,
     private _internalStateOperations: InternalStateOperations,
@@ -43,7 +61,7 @@ export class Store {
   select<T>(selector: StateToken<T>): Observable<T>;
   select(selector: any): Observable<any> {
     const selectorFn = this.getStoreBoundSelectorFn(selector);
-    return this._stateStream.pipe(
+    return this._selectableStateStream.pipe(
       map(selectorFn),
       catchError((err: Error): Observable<never> | Observable<undefined> => {
         // if error is TypeError we swallow it to prevent usual errors with property access
@@ -87,7 +105,9 @@ export class Store {
    * Allow the user to subscribe to the root of the state
    */
   subscribe(fn?: (value: any) => void): Subscription {
-    return this._stateStream.pipe(leaveNgxs(this._internalExecutionStrategy)).subscribe(fn);
+    return this._selectableStateStream
+      .pipe(leaveNgxs(this._internalExecutionStrategy))
+      .subscribe(fn);
   }
 
   /**

diff --git a/packages/store/tests/issues/issue-933-selectors-causing-ticks.spec.ts b/packages/store/tests/issues/issue-933-selectors-causing-ticks.spec.ts
@@ -7,6 +7,8 @@ import { freshPlatform, skipConsoleLogging } from '@ngxs/store/internals/testing
 describe('Selectors within templates causing ticks (https://github.com/ngxs/store/issues/933)', () => {
   class SetCountries {
     static readonly type = '[CountriesState] Set countries';
+
+    constructor(public countries: string[]) {}
   }
 
   @State<string[]>({
@@ -15,14 +17,12 @@ describe('Selectors within templates causing ticks (https://github.com/ngxs/stor
   })
   class CountriesState {
     @Action(SetCountries)
-    async setCountries(ctx: StateContext<string[]>) {
+    async setCountries(ctx: StateContext<string[]>, action: SetCountries) {
       await Promise.resolve();
-      ctx.setState(['USA', 'Canada']);
+      ctx.setState(action.countries);
     }
   }
 
-  const count = 10;
-
   @Component({
     selector: 'app-child',
     template: `
@@ -42,7 +42,7 @@ describe('Selectors within templates causing ticks (https://github.com/ngxs/stor
     `
   })
   class TestComponent {
-    items = new Array(count);
+    items = new Array(10);
   }
 
   @NgModule({
@@ -53,7 +53,7 @@ describe('Selectors within templates causing ticks (https://github.com/ngxs/stor
   class TestModule {}
 
   it(
-    'should run change detection whenever the selector emits after asynchronous action has been completed',
+    'should run change detection once for all selectors when asynchronous action has been completed',
     freshPlatform(async () => {
       // Arrange
       const { injector } = await skipConsoleLogging(() =>
@@ -62,16 +62,67 @@ describe('Selectors within templates causing ticks (https://github.com/ngxs/stor
       const store = injector.get(Store);
       const appRef = injector.get(ApplicationRef);
       const spy = jest.spyOn(appRef, 'tick');
+      const children = document.querySelectorAll('app-child');
 
       // Act
-      await store.dispatch(new SetCountries()).toPromise();
+      await store.dispatch(new SetCountries(['USA', 'Canada'])).toPromise();
 
       // Assert
       try {
-        expect(spy.mock.calls.length).toBeGreaterThan(count);
+        children.forEach(child => {
+          expect(child.innerHTML).toContain('USA,Canada');
+        });
+
+        expect(spy).toHaveBeenCalledTimes(3);
       } finally {
         spy.mockRestore();
       }
     })
   );
+
+  it(
+    '`store.select` should emit state changes and should emit the latest value even if there are no subscription (since the `refCount()` is used)',
+    freshPlatform(async () => {
+      // Arrange
+      const { injector } = await skipConsoleLogging(() =>
+        platformBrowserDynamic().bootstrapModule(TestModule)
+      );
+      const store = injector.get(Store);
+      const recordedCountries: string[][] = [];
+
+      // Act
+      let subscription = store.select(CountriesState).subscribe(countries => {
+        recordedCountries.push(countries);
+      });
+
+      await store.dispatch(new SetCountries(['USA'])).toPromise();
+
+      // Assert
+      expect(recordedCountries).toEqual([[], ['USA']]);
+
+      // Act
+      subscription.unsubscribe();
+      recordedCountries.length = 0;
+
+      await store.dispatch(new SetCountries(['Canada'])).toPromise();
+      await store.dispatch(new SetCountries(['Mexico'])).toPromise();
+
+      subscription = store.select(CountriesState).subscribe(countries => {
+        recordedCountries.push(countries);
+      });
+
+      // Assert
+      expect(recordedCountries).toEqual([['Mexico']]);
+      expect(store.selectSnapshot(CountriesState)).toEqual(['Mexico']);
+
+      // Act
+      await store.dispatch(new SetCountries(['Salvador'])).toPromise();
+
+      // Assert
+      expect(recordedCountries).toEqual([['Mexico'], ['Salvador']]);
+      expect(store.selectSnapshot(CountriesState)).toEqual(['Salvador']);
+
+      subscription.unsubscribe();
+    })
+  );
 });
diff --git a/packages/store/tests/zone.spec.ts b/packages/store/tests/zone.spec.ts
@@ -2,7 +2,7 @@ import { ApplicationRef, Component, Injectable, NgModule, NgZone } from '@angula
 import { TestBed } from '@angular/core/testing';
 import { BrowserModule } from '@angular/platform-browser';
 import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';
-import { freshPlatform } from '@ngxs/store/internals/testing';
+import { freshPlatform, skipConsoleLogging } from '@ngxs/store/internals/testing';
 import { take } from 'rxjs/operators';
 
 import { Action, NgxsModule, State, StateContext, Store } from '../src/public_api';
@@ -25,53 +25,40 @@ describe('zone', () => {
     }
   }
 
-  describe('[store.select]', () => {
-    it('should be performed inside Angular zone', () => {
-      let ticks = 0;
-
-      class MockApplicationRef extends ApplicationRef {
-        public tick(): void {
-          ticks++;
-        }
-      }
-
-      TestBed.configureTestingModule({
-        imports: [NgxsModule.forRoot([CounterState])],
-        providers: [
-          {
-            provide: ApplicationRef,
-            useClass: MockApplicationRef
-          }
-        ]
-      });
-
-      const store: Store = TestBed.inject(Store);
-      const zone: NgZone = TestBed.inject(NgZone);
+  // =============================================================
+  it('"store.subscribe" should be performed inside Angular zone', () => {
+    let nextCalls = 0;
+    let nextCallsInAngularZone = 0;
 
-      // NGXS performes initializions inside Angular zone
-      // thus it causes app to tick
-      expect(ticks).toBeGreaterThan(0);
+    TestBed.configureTestingModule({
+      imports: [NgxsModule.forRoot([CounterState])]
+    });
 
-      zone.runOutsideAngular(() => {
-        store
-          .select<number>(({ counter }) => counter)
-          .pipe(take(3))
-          .subscribe(() => {
-            expect(NgZone.isInAngularZone()).toBeTruthy();
-          });
+    const store: Store = TestBed.inject(Store);
+    const ngZone: NgZone = TestBed.inject(NgZone);
 
-        store.dispatch(new Increment());
-        store.dispatch(new Increment());
+    ngZone.runOutsideAngular(() => {
+      store.subscribe(() => {
+        nextCalls++;
+        if (NgZone.isInAngularZone()) {
+          nextCallsInAngularZone++;
+        }
       });
 
-      // Angular has run change detection 5 times
-      expect(ticks).toBe(5);
+      store.dispatch(new Increment());
+      store.dispatch(new Increment());
     });
+
+    expect(nextCalls).toEqual(3);
+    expect(nextCallsInAngularZone).toEqual(3);
   });
+  // =============================================================
 
   // =============================================================
   it('"select" should be performed inside Angular zone', () => {
     let ticks = 0;
+    let selectCalls = 0;
+    let selectCallsInAngularZone = 0;
 
     class MockApplicationRef extends ApplicationRef {
       public tick(): void {
@@ -94,14 +81,17 @@ describe('zone', () => {
 
     // NGXS performes initializions inside Angular zone
     // thus it causes app to tick
-    expect(ticks).toBeGreaterThan(0);
+    expect(ticks).toEqual(2);
 
     zone.runOutsideAngular(() => {
       store
         .select<number>(({ counter }) => counter)
         .pipe(take(3))
         .subscribe(() => {
-          expect(NgZone.isInAngularZone()).toBeTruthy();
+          selectCalls++;
+          if (NgZone.isInAngularZone()) {
+            selectCallsInAngularZone++;
+          }
         });
 
       store.dispatch(new Increment());
@@ -110,6 +100,8 @@ describe('zone', () => {
 
     // Angular has run change detection 5 times
     expect(ticks).toBe(5);
+    expect(selectCalls).toEqual(3);
+    expect(selectCallsInAngularZone).toEqual(3);
   });
 
   it('"select" should be performed outside Angular zone', () => {
@@ -154,6 +146,7 @@ describe('zone', () => {
     // Angular hasn't run any change detection
     expect(ticks).toBe(0);
   });
+  // =============================================================
 
   it('action should be handled inside zone if NoopNgxsExecutionStrategy is used', () => {
     class FooAction {
@@ -246,9 +239,9 @@ describe('zone', () => {
       })
       class MockModule {}
 
-      const { injector } = await platformBrowserDynamic().bootstrapModule(MockModule, {
-        ngZone: 'noop'
-      });
+      const { injector } = await skipConsoleLogging(() =>
+        platformBrowserDynamic().bootstrapModule(MockModule, { ngZone: 'noop' })
+      );
       const store = injector.get<Store>(Store);
       store.dispatch(new FooAction());
     })