refactor: add checks to prevent prev/state property values directly

.gitignore

@@ -4,6 +4,7 @@
 
 .rollup.cache
 .yarn/cache
+.yarn/versions
 .yarn/install-state.gz
 
 lib/

packages/core/src/DispatchManager.ts

@@ -27,28 +27,51 @@ export abstract class DispatchManager<State> extends HistoryManager<State> {
     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<State> extends HistoryManager<State> {
 
         this.setState(
           actionHandler({
-            state: this.getState(),
+            state: this.state,
             payload,
           })
         );

packages/core/src/HandlerManager.ts

@@ -52,34 +52,7 @@ export abstract class HandlerManager<
   protected getHandler(dispatcher: AnyDispatcher<Handler>) {
     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}'`

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<State> 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<State> 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<State> 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.
    */

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<number> {
+      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<number> {
+      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);