diff --git a/TODO.md b/TODO.md index e952a61c..015edcc3 100644 --- a/TODO.md +++ b/TODO.md @@ -2,3 +2,6 @@ - landing page to link to all API docs - coveralls - help may be here, to combine multiple coverage runs into one report: https://github.com/angular/angular-cli/issues/11268 + +# App State +- investigate unsubscribe performance in the `wide-performance.ts` diff --git a/projects/app-state/README.md b/projects/app-state/README.md index f8a0b302..3ee4c0fb 100644 --- a/projects/app-state/README.md +++ b/projects/app-state/README.md @@ -193,5 +193,6 @@ If you are upgrading from the loose version of [`ng-app-state`](https://github.c - Replace references to `AppStore` with `RootStore`. - The constructor for `RootStore` takes only 1 argument: its starting state. When transitioning from `AppStore` to `RootStore`, remove the other 2 arguments. - Remove calls to `withCaching()`. The redesigned library comes with intelligent caching built in automatically. -- Batches are no longer tied to a specific slice of the store, but always operate on the entire `RootStore`. Therefore, the callback to `batch()` no longer receives a `Store` object and `.inBatch()` has been removed. Instead, operate on the store using any existing store object and it will automatically be included in the batch. +- Batches are no longer tied to a specific slice of the store, but always operate on the entire `RootStore`. Therefore, `.batch()` was moved to `RootStore` and its callback no longer receives a `Store` object. Instead, operate on the store using any existing store object and it will automatically be included in the batch. `Store.getRootStore()` was added to get a reference to start the batch from if needed. +- `.inBatch()` has been removed. - The `batch` parameter was removed from `UndoManager.applyUndoState()`. It still operates in a batch, but now you can use `this.store` in its place with no need for the additional argument. diff --git a/projects/app-state/src/lib/child-store.ts b/projects/app-state/src/lib/child-store.ts index d257222e..a00e6b91 100644 --- a/projects/app-state/src/lib/child-store.ts +++ b/projects/app-state/src/lib/child-store.ts @@ -1,10 +1,14 @@ import { clone, isEmpty, omit } from '@s-libs/micro-dash'; -import { Client, Store } from './index'; +import { RootStore, Store } from './index'; /** @hidden */ export class ChildStore extends Store { - constructor(client: Client, private parent: Store, private key: any) { - super(client); + constructor( + getRootStore: () => RootStore, + private parent: Store, + private key: any, + ) { + super(getRootStore); } set(value: T): void { @@ -36,8 +40,8 @@ export class ChildStore extends Store { refersToSameStateAs(other: Store): boolean { return ( other instanceof ChildStore && - this.key === (other as ChildStore).key && - this.parent.refersToSameStateAs((other as ChildStore).parent) + this.key === other.key && + this.parent.refersToSameStateAs(other.parent) ); } diff --git a/projects/app-state/src/lib/index.ts b/projects/app-state/src/lib/index.ts index f5fe1c75..e2cbae5c 100644 --- a/projects/app-state/src/lib/index.ts +++ b/projects/app-state/src/lib/index.ts @@ -1,4 +1,4 @@ // this exists to resolve the order of some circular imports -export { Client, Store } from './store'; +export { Store } from './store'; export { RootStore } from './root-store'; export { ChildStore } from './child-store'; diff --git a/projects/app-state/src/lib/root-store.spec.ts b/projects/app-state/src/lib/root-store.spec.ts index ddba0e12..70a0ef75 100644 --- a/projects/app-state/src/lib/root-store.spec.ts +++ b/projects/app-state/src/lib/root-store.spec.ts @@ -1,3 +1,4 @@ +import { noop } from '@s-libs/micro-dash'; import { InnerState, TestState } from '../test-helpers/test-state'; import { RootStore } from './root-store'; @@ -61,6 +62,85 @@ describe('RootStore', () => { }); }); + describe('.batch()', () => { + it('causes a single update after multiple actions', () => { + const next = jasmine.createSpy(); + + store.$.subscribe(next); + expect(next).toHaveBeenCalledTimes(1); + + store.batch(() => { + store('counter').set(3); + store('nested')('state').set(6); + expect(next).toHaveBeenCalledTimes(1); + }); + + expect(next).toHaveBeenCalledTimes(2); + expect(store.state()).toEqual({ counter: 3, nested: { state: 6 } }); + }); + + it('works when nested', () => { + store.batch(() => { + store('counter').set(1); + store.batch(() => { + expect(store.state().counter).toBe(1); + store('counter').set(2); + expect(store.state().counter).toBe(2); + }); + expect(store.state().counter).toBe(2); + }); + expect(store.state().counter).toBe(2); + }); + + it("doesn't have that infinite loop with 2 stores (production bug)", () => { + // https://github.com/simontonsoftware/ng-app-state/issues/28 + const store2 = new RootStore<{}>({}); + store.$.subscribe(() => { + store2.batch(noop); + }); + store2.$.subscribe(); + store('counter').set(1); + + // the infinite loop was here + + expect(store.state().counter).toBe(1); + }); + + it("doesn't have that infinite loop with batches (production bug)", () => { + // https://github.com/simontonsoftware/ng-app-state/issues/29 + class State { + version = 1; + looping = false; + } + const loopStore = new RootStore(new State()); + loopStore('version').$.subscribe(() => { + loopStore.batch(noop); + }); + loopStore('looping').$.subscribe(() => { + loopStore('version').set(2); + loopStore('version').set(3); + }); + + loopStore('looping').set(true); + // the infinite loop was here + + expect(loopStore.state().version).toBe(3); + }); + + it('starts nested batches with the correct state (production bug)', () => { + store.batch(() => { + store('counter').set(1); + store.batch(() => { + expect(store.state().counter).toBe(1); + store('nested')('state').set(2); + }); + }); + expect(store.state()).toEqual( + jasmine.objectContaining({ counter: 1, nested: { state: 2 } }), + ); + }); + }); + describe('.refersToSameStateAs()', () => { it('works', () => { expect(store.refersToSameStateAs(store)).toBe(true); diff --git a/projects/app-state/src/lib/root-store.ts b/projects/app-state/src/lib/root-store.ts index 8a0c71ed..5715e773 100644 --- a/projects/app-state/src/lib/root-store.ts +++ b/projects/app-state/src/lib/root-store.ts @@ -4,18 +4,7 @@ export class RootStore extends Store { private batchCount = 0; constructor(state: T) { - super({ - runInBatch: (func: () => void) => { - ++this.batchCount; - try { - func(); - } finally { - if (--this.batchCount === 0) { - this.maybeEmit(); - } - } - }, - }); + super(() => this); this.set(state); } @@ -32,6 +21,30 @@ export class RootStore extends Store { return this.lastKnownState!; } + /** + * Turns off this store's observables while `func` is executing, emitting only once afterward, if the store changed. This allows you to batch multiple mutations into a single update at the end. + * + * ```ts + * store.batch(() => { + * store.assign({ key1: value1 }); + * store('key2').delete(); + * store('key3').set({ key4: value4 }); + * + * store('key1').state(); // returns `value1` + * }); + * ``` + */ + batch(func: VoidFunction): void { + ++this.batchCount; + try { + func(); + } finally { + if (--this.batchCount === 0) { + this.maybeEmit(); + } + } + } + refersToSameStateAs(other: Store): boolean { return this === other; } diff --git a/projects/app-state/src/lib/store.spec.ts b/projects/app-state/src/lib/store.spec.ts index baaa13b5..294662e8 100644 --- a/projects/app-state/src/lib/store.spec.ts +++ b/projects/app-state/src/lib/store.spec.ts @@ -1,4 +1,4 @@ -import { cloneDeep, identity, noop, pick } from '@s-libs/micro-dash'; +import { cloneDeep, identity, pick } from '@s-libs/micro-dash'; import { expectSingleCallAndReset } from '@s-libs/ng-dev'; import { skip, take } from 'rxjs/operators'; import { InnerState, TestState } from '../test-helpers/test-state'; @@ -193,85 +193,6 @@ describe('Store', () => { }); }); - describe('.batch()', () => { - it('causes a single update after multiple actions', () => { - const next = jasmine.createSpy(); - - store.$.subscribe(next); - expect(next).toHaveBeenCalledTimes(1); - - store.batch(() => { - store('counter').set(3); - store('nested')('state').set(6); - expect(next).toHaveBeenCalledTimes(1); - }); - - expect(next).toHaveBeenCalledTimes(2); - expect(store.state()).toEqual({ counter: 3, nested: { state: 6 } }); - }); - - it('works when nested', () => { - store.batch(() => { - store('counter').set(1); - store.batch(() => { - expect(store.state().counter).toBe(1); - store('counter').set(2); - expect(store.state().counter).toBe(2); - }); - expect(store.state().counter).toBe(2); - }); - expect(store.state().counter).toBe(2); - }); - - it("doesn't have that infinite loop with 2 stores (production bug)", () => { - // https://github.com/simontonsoftware/ng-app-state/issues/28 - const store2 = new RootStore<{}>({}); - store.$.subscribe(() => { - store2.batch(noop); - }); - store2.$.subscribe(); - store('counter').set(1); - - // the infinite loop was here - - expect(store.state().counter).toBe(1); - }); - - it("doesn't have that infinite loop with batches (production bug)", () => { - // https://github.com/simontonsoftware/ng-app-state/issues/29 - class State { - version = 1; - looping = false; - } - const loopStore = new RootStore(new State()); - loopStore('version').$.subscribe(() => { - loopStore.batch(noop); - }); - loopStore('looping').$.subscribe(() => { - loopStore('version').set(2); - loopStore('version').set(3); - }); - - loopStore('looping').set(true); - // the infinite loop was here - - expect(loopStore.state().version).toBe(3); - }); - - it('starts nested batches with the correct state (production bug)', () => { - store.batch(() => { - store('counter').set(1); - store.batch(() => { - expect(store.state().counter).toBe(1); - store('nested')('state').set(2); - }); - }); - expect(store.state()).toEqual( - jasmine.objectContaining({ counter: 1, nested: { state: 2 } }), - ); - }); - }); - describe('.assign()', () => { it('assigns the exact objects given', () => { const before = store.state().nested; @@ -401,6 +322,13 @@ describe('Store', () => { }); }); + describe('.getRootStore()', () => { + it('returns a reference to the root store', () => { + expect(store.getRootStore()).toBe(store); + expect(store('nested')('state').getRootStore()).toBe(store); + }); + }); + describe('help for subclasses', () => { it('does nothing when trying to update to the same value', () => { const startingState = store.state(); diff --git a/projects/app-state/src/lib/store.ts b/projects/app-state/src/lib/store.ts index a053d816..db10c46b 100644 --- a/projects/app-state/src/lib/store.ts +++ b/projects/app-state/src/lib/store.ts @@ -1,12 +1,7 @@ import { CallableObject } from '@s-libs/js-core'; import { clone, every, forOwn } from '@s-libs/micro-dash'; import { Observable, Subscriber } from 'rxjs'; -import { ChildStore } from './index'; - -/** @hidden */ -export interface Client { - runInBatch(func: () => void): void; -} +import { ChildStore, RootStore } from './index'; /** @hidden */ type GetSlice = (attr: K) => Store; @@ -35,16 +30,16 @@ export abstract class Store extends CallableObject> { }); protected subscribers = new Map, T | undefined>(); - protected activeChildren: Record>> = {}; + protected activeChildren: Record>> = {}; protected lastKnownState?: T; private lastKnownStateChanged = false; - constructor(private client: Client) { + constructor(public getRootStore: () => RootStore) { super( (childKey: any) => this.activeChildren[childKey]?.values().next()?.value || - new ChildStore(client, this, childKey), + new ChildStore(getRootStore, this, childKey), ); } @@ -72,24 +67,6 @@ export abstract class Store extends CallableObject> { */ abstract refersToSameStateAs(other: Store): boolean; - /** - * Allows batching multiple mutations on this store object so that observers only receive one event. The batch maintains its own fork of the full global state until it completes, then commits it to the store. Calls to `.state()` on the batch will fetch from the forked state. - * - * ```ts - * store.batch((batch) => { - * batch.assign({ key1: value1 }); - * batch('key2').delete(); - * batch('key3').set({ key4: value4 }); - * - * batch('key1').state(); // returns `value1` - * store('key1').state(); // don't do this. may not return `value1` - * }); - * ``` - */ - batch(func: () => void): void { - this.client.runInBatch(func); - } - /** * Assigns the given values to state of this store object. The resulting state will be like `Object.assign(store.state(), value)`. */ diff --git a/projects/app-state/src/lib/utils/push-to-store-array.spec.ts b/projects/app-state/src/lib/utils/push-to-store-array.spec.ts index 6a35031c..1d176243 100644 --- a/projects/app-state/src/lib/utils/push-to-store-array.spec.ts +++ b/projects/app-state/src/lib/utils/push-to-store-array.spec.ts @@ -36,7 +36,7 @@ describe('pushToStoreArray', () => { let so1: Store; let so2: Store; - store.batch(() => { + store.getRootStore().batch(() => { so1 = pushToStoreArray(store, 1); so2 = pushToStoreArray(store, 2); diff --git a/projects/app-state/src/lib/utils/undo-manager.ts b/projects/app-state/src/lib/utils/undo-manager.ts index 8fb3feb7..a62574fa 100644 --- a/projects/app-state/src/lib/utils/undo-manager.ts +++ b/projects/app-state/src/lib/utils/undo-manager.ts @@ -189,7 +189,7 @@ export abstract class UndoManager { const stateToOverwrite = this.currentUndoState; this.currentStateIndex += change; const stateToApply = this.currentUndoState; - this.store.batch(() => { + this.store.getRootStore().batch(() => { this.applyUndoState(stateToApply, undoOrRedo, stateToOverwrite); }); this.emitUndoChanges(); diff --git a/projects/app-state/src/typing-tests/store-object.dts-spec.ts b/projects/app-state/src/typing-tests/store.dts-spec.ts similarity index 76% rename from projects/app-state/src/typing-tests/store-object.dts-spec.ts rename to projects/app-state/src/typing-tests/store.dts-spec.ts index 51cc5b0b..7c93192e 100644 --- a/projects/app-state/src/typing-tests/store-object.dts-spec.ts +++ b/projects/app-state/src/typing-tests/store.dts-spec.ts @@ -19,3 +19,8 @@ store('obj')('c'); store('ary'); // $ExpectType Store store('ary')(1); + +// $ExpectType RootStore +store.getRootStore(); +// $ExpectType RootStore +store('obj')('c').getRootStore();