diff --git a/.gitignore b/.gitignore index ba234fc..913e3ae 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ .rollup.cache .yarn/cache +.yarn/versions .yarn/install-state.gz lib/ diff --git a/packages/core/src/DispatchManager.ts b/packages/core/src/DispatchManager.ts index 4e8648a..832d7de 100644 --- a/packages/core/src/DispatchManager.ts +++ b/packages/core/src/DispatchManager.ts @@ -27,28 +27,51 @@ export abstract class DispatchManager extends HistoryManager { const payload = args[0]; const type = dispatcher.type; const name = dispatcher.displayName; + const storeId = dispatcher.storeId; + const isGlobal = !dispatcher.parent; - const handler = this.getHandler(dispatcher) as DispatchHandler; + /** + * This check is to ensure that the dispatcher is not fired from a store + * that it is not registered with. + * + * This is a runtime check and will throw an error in production as well. The + * expectation is that the consumer will consume this error and fix it in their + * codebase. + * + * Why is this needed? Consider an example with two stores `StoreA` and `StoreB`. + * and an action `increment` that is registered with both `StoreA` and `StoreB`. This check + * will ensure that `increment` registered with `StoreA` is only fired from `StoreA`, thus + * preventing any side-effects that might occur if a wrong action is fired from wrong store. + */ + if (this.id !== storeId) { + const prefix = type === Dispatcher.ACTION ? "Action" : "Effect"; - const isGlobal = !dispatcher.parent; + throw new RangeError( + `${prefix} '${name}' cannot be fired from store '${this.tag}'.` + ); + } + + const handler = this.getHandler(dispatcher) as DispatchHandler; /** - * Since `DispatchManager` extends `@HistoryManager`, we use the `trace` + * Since `DispatchManager` extends `HistoryManager`, we use the `trace` * method to create a trace of the dispatch. */ const trace = this.trace({ - global: isGlobal, name, type, payload, ...(!isGlobal ? { + global: false, source: { name: dispatcher.parent?.displayName, type: dispatcher.parent?.type, }, } - : {}), + : { + global: true, + }), }); switch (type) { @@ -61,7 +84,7 @@ export abstract class DispatchManager extends HistoryManager { this.setState( actionHandler({ - state: this.getState(), + state: this.state, payload, }) ); diff --git a/packages/core/src/HandlerManager.ts b/packages/core/src/HandlerManager.ts index ccdd027..96e509f 100644 --- a/packages/core/src/HandlerManager.ts +++ b/packages/core/src/HandlerManager.ts @@ -52,34 +52,7 @@ export abstract class HandlerManager< protected getHandler(dispatcher: AnyDispatcher) { const type = dispatcher.type; const name = dispatcher.displayName; - const storeId = dispatcher.storeId; - /** - * This check is to ensure that the dispatcher is not fired from a store - * that it is not registered with. - * - * This is a runtime check and will throw an error in production as well. The - * expectation is that the consumer will consume this error and fix it in their - * codebase. - * - * Why is this here? Consider an example with two stores `StoreA` and `StoreB`. - * and an action `increment` that is registered with both `StoreA` and `StoreB`. This check - * will ensure that `increment` registered with `StoreA` is only fired from `StoreA`, thus - * preventing any side-effects that might occur if a wrong action is fired from wrong store. - */ - if (this.id !== storeId) { - const prefix = type === Dispatcher.ACTION ? "Action" : "Effect"; - - throw new RangeError( - `${prefix} '${name}' cannot be fired from store '${this.tag}'.` - ); - } - - /** - * This check is to ensure that the dispatcher is registered with the store. - * The error thrown here is an expected behaviour and the expectation is that - * the consumer will consume this error and fix it in their codebase. - */ if (!this.#dispatchers.has(`${type}-${name}`)) { throw new TypeError( `Dispatcher ${type} ${name} is not registered with the store '${this.tag}'` diff --git a/packages/core/src/StateManager.ts b/packages/core/src/StateManager.ts index ba57636..37c94c0 100644 --- a/packages/core/src/StateManager.ts +++ b/packages/core/src/StateManager.ts @@ -7,6 +7,9 @@ import { IdManager } from "./IdManager"; * extended and provides the necessary methods to interact with the state. */ export abstract class StateManager extends IdManager { + /** + * Only mutate this directly within `setState` method. Otherwise + */ #state: State; #previousStates: State[]; #subscribers: ((state: State) => void)[] = []; @@ -18,34 +21,23 @@ export abstract class StateManager extends IdManager { } /** - * Set the new state. We do not provide any validation here as it is - * the responsibility of the consumer. - */ - protected setState(state: State) { - this.#previousStates.unshift(this.#state); - this.#state = state; - - /** - * @todo(samrith-s): Find a better way to handel subscribers, maybe use a queue. - */ - this.#subscribers.forEach((callback) => callback(state)); - } - - /** - * We have this method separately to make it easier to access the - * state in the classes that inherit from `StateManager`. - * + * We have this property separately to make it easier to access the + * state in the classes that inherit `StateManager`. */ protected get state() { - return this.#state; + return this.getState(); } /** - * The `getState` method is used to get the current state. + * Never allow setting state directly via this property. It should only be + * mutated via the `setState` method to ensure any optimisations or features + * we might add while updating state will be centrally managed. */ - public getState = () => { - return this.#state; - }; + protected set state(_value: State) { + throw new Error( + "You cannot set the state directly. Use the `setState` method instead." + ); + } /** * We have this method separately to make it easier to access the @@ -58,6 +50,42 @@ export abstract class StateManager extends IdManager { return this.#previousStates[0]; } + /** + * Never allow setting previous state directly. It is populated internally as + * a side effect of setting the state. + */ + protected set previousState(_value: State) { + throw new Error( + "You cannot set the previous state directly. It is managed internally." + ); + } + + /** + * Set the new state. We do not provide any validation here as it is + * the responsibility of the consumer. + */ + protected setState(state: State) { + this.#previousStates.unshift(this.#state); + this.#state = state; + + /** + * @todo(samrith-s): Find a better way to handel subscribers, maybe use a queue. + */ + this.#subscribers.forEach((callback) => callback(state)); + } + + /** + * The `getState` method is used to get the current state. + */ + public getState() { + /** + * Always returning a sealed object to prevent accidental mutation. + * + * @todo(samrith-s): Is this really necessary? + */ + return Object.seal(this.#state); + } + /** * The `getPreviousStates` returns all the previous states in the lifetime of the store. */ diff --git a/packages/core/src/__tests__/state.test.ts b/packages/core/src/__tests__/state.test.ts index fb7bad2..d502faa 100644 --- a/packages/core/src/__tests__/state.test.ts +++ b/packages/core/src/__tests__/state.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from "vitest"; +import { StateManager } from "../StateManager"; import { Store } from "../Store"; describe("state", () => { @@ -8,6 +9,16 @@ describe("state", () => { expect(store.getState()).toBe(10); }); + it("should not allow setting state in any way except through `setState`", () => { + const sm = new (class extends StateManager { + public dummySetter(value: number) { + this.state = value; + } + })(10); + + expect(() => sm.dummySetter(10)).toThrowError(); + }); + it("should print previous state correctly", () => { const store = new Store(10); const action = store.action("increment", ({ state }) => state + 10); @@ -18,6 +29,16 @@ describe("state", () => { expect(store.getPreviousStates()).toStrictEqual([10, 10]); }); + it("should not allow setting previous state in any way via inherited classes", () => { + const sm = new (class extends StateManager { + public dummySetter(value: number) { + this.previousState = value; + } + })(10); + + expect(() => sm.dummySetter(10)).toThrowError(); + }); + it("should emit an event when state changes", () => { const store = new Store(10); const action = store.action("increment", ({ state }) => state + 10);