From 4699711cbd8962cd777bee49a7593420f87c37f6 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Wed, 3 Mar 2021 13:21:05 -0330 Subject: [PATCH 1/6] Add controller messaging system Adds a controller messaging system, which will be used to facilitate inter-controller communication. The controller messenger acts as a message broker, passing actions and events back and forth between controllers. It is _fully type safe_. This idea was described in the Controller Messaging System proposal[1]. This controller messenger doesn't yet support attenuation, so it's not yet possible to restrict which actions and events are interacted with. That will be coming in a later PR. [1]: https://www.notion.so/Controller-Messaging-System-617efb02b9e54bd0a0b0e44c6f776d85 --- src/BaseControllerV2.test.ts | 118 ++++++++++--- src/BaseControllerV2.ts | 28 +++- src/ControllerMessenger.test.ts | 287 ++++++++++++++++++++++++++++++++ src/ControllerMessenger.ts | 89 ++++++++++ 4 files changed, 493 insertions(+), 29 deletions(-) create mode 100644 src/ControllerMessenger.test.ts create mode 100644 src/ControllerMessenger.ts diff --git a/src/BaseControllerV2.test.ts b/src/BaseControllerV2.test.ts index dd1161497c1..c5bd062e411 100644 --- a/src/BaseControllerV2.test.ts +++ b/src/BaseControllerV2.test.ts @@ -1,21 +1,27 @@ -import type { Draft } from 'immer'; +import type { Draft, Patch } from 'immer'; import * as sinon from 'sinon'; import { BaseController, getAnonymizedState, getPersistentState } from './BaseControllerV2'; +import { ControllerMessenger } from './ControllerMessenger'; -type MockControllerState = { +type CountControllerState = { count: number; }; -const mockControllerStateMetadata = { +type CountControllerEvent = { + type: `CountController:state-change`; + payload: [CountControllerState, Patch[]]; +}; + +const CountControllerStateMetadata = { count: { persist: true, anonymous: true, }, }; -class MockController extends BaseController { - update(callback: (state: Draft) => void | MockControllerState) { +class MockController extends BaseController<'CountController', CountControllerState> { + update(callback: (state: Draft) => void | CountControllerState) { super.update(callback); } @@ -26,19 +32,37 @@ class MockController extends BaseController { describe('BaseController', () => { it('should set initial state', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); expect(controller.state).toEqual({ count: 0 }); }); it('should set initial schema', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); - expect(controller.metadata).toEqual(mockControllerStateMetadata); + expect(controller.metadata).toEqual(CountControllerStateMetadata); }); it('should not allow mutating state directly', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); expect(() => { controller.state = { count: 1 }; @@ -46,7 +70,13 @@ describe('BaseController', () => { }); it('should allow updating state by modifying draft', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); controller.update((draft) => { draft.count += 1; @@ -56,7 +86,13 @@ describe('BaseController', () => { }); it('should allow updating state by return a value', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); controller.update(() => { return { count: 1 }; @@ -66,7 +102,13 @@ describe('BaseController', () => { }); it('should throw an error if update callback modifies draft and returns value', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); expect(() => { controller.update((draft) => { @@ -77,7 +119,13 @@ describe('BaseController', () => { }); it('should inform subscribers of state changes', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); const listener1 = sinon.stub(); const listener2 = sinon.stub(); @@ -94,7 +142,13 @@ describe('BaseController', () => { }); it('should inform a subscriber of each state change once even after multiple subscriptions', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); const listener1 = sinon.stub(); controller.subscribe(listener1); @@ -108,7 +162,13 @@ describe('BaseController', () => { }); it('should no longer inform a subscriber about state changes after unsubscribing', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); const listener1 = sinon.stub(); controller.subscribe(listener1); @@ -121,7 +181,13 @@ describe('BaseController', () => { }); it('should no longer inform a subscriber about state changes after unsubscribing once, even if they subscribed many times', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); const listener1 = sinon.stub(); controller.subscribe(listener1); @@ -134,17 +200,29 @@ describe('BaseController', () => { expect(listener1.callCount).toEqual(0); }); - it('should allow unsubscribing listeners who were never subscribed', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + it('should throw when unsubscribing listener who was never subscribed', () => { + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); const listener1 = sinon.stub(); expect(() => { controller.unsubscribe(listener1); - }).not.toThrow(); + }).toThrow(); }); it('should no longer update subscribers after being destroyed', () => { - const controller = new MockController({ count: 0 }, mockControllerStateMetadata); + const controllerMessenger = new ControllerMessenger(); + const controller = new MockController( + controllerMessenger, + 'CountController', + { count: 0 }, + CountControllerStateMetadata, + ); const listener1 = sinon.stub(); const listener2 = sinon.stub(); diff --git a/src/BaseControllerV2.ts b/src/BaseControllerV2.ts index 6ece6f31412..2365b431445 100644 --- a/src/BaseControllerV2.ts +++ b/src/BaseControllerV2.ts @@ -4,6 +4,8 @@ import { enablePatches, produceWithPatches } from 'immer'; // eslint-disable-next-line no-duplicate-imports import type { Draft, Patch } from 'immer'; +import type { ControllerMessenger } from './ControllerMessenger'; + enablePatches(); /** @@ -96,21 +98,31 @@ type Json = null | boolean | number | string | Json[] | { [prop: string]: Json } /** * Controller class that provides state management, subscriptions, and state metadata */ -export class BaseController> { +export class BaseController> { private internalState: IsJsonable; - private internalListeners: Set> = new Set(); + private messagingSystem: ControllerMessenger; + + private name: N; public readonly metadata: StateMetadata; /** * Creates a BaseController instance. * + * @param messagingSystem - Controller messaging system * @param state - Initial controller state * @param metadata - State metadata, describing how to "anonymize" the state, * and which parts should be persisted. */ - constructor(state: IsJsonable, metadata: StateMetadata) { + constructor( + messagingSystem: ControllerMessenger, + name: N, + state: IsJsonable, + metadata: StateMetadata, + ) { + this.messagingSystem = messagingSystem; + this.name = name; this.internalState = state; this.metadata = metadata; } @@ -134,7 +146,7 @@ export class BaseController> { * @param listener - Callback triggered when state changes */ subscribe(listener: Listener) { - this.internalListeners.add(listener); + this.messagingSystem.subscribe(`${this.name}:state-change` as `${N}:state-change`, listener); } /** @@ -143,7 +155,7 @@ export class BaseController> { * @param listener - Callback to remove */ unsubscribe(listener: Listener) { - this.internalListeners.delete(listener); + this.messagingSystem.unsubscribe(`${this.name}:state-change` as `${N}:state-change`, listener); } /** @@ -158,9 +170,7 @@ export class BaseController> { protected update(callback: (state: Draft>) => void | IsJsonable) { const [nextState, patches] = produceWithPatches(this.internalState, callback); this.internalState = nextState as IsJsonable; - for (const listener of this.internalListeners) { - listener(nextState as S, patches); - } + this.messagingSystem.publish(`${this.name}:state-change` as `${N}:state-change`, nextState as S, patches); } /** @@ -173,7 +183,7 @@ export class BaseController> { * listeners from being garbage collected. */ protected destroy() { - this.internalListeners.clear(); + this.messagingSystem.clearEventSubscriptions(`${this.name}:state-change` as `${N}:state-change`); } } diff --git a/src/ControllerMessenger.test.ts b/src/ControllerMessenger.test.ts new file mode 100644 index 00000000000..672c853d474 --- /dev/null +++ b/src/ControllerMessenger.test.ts @@ -0,0 +1,287 @@ +import * as sinon from 'sinon'; + +import { ControllerMessenger } from './ControllerMessenger'; + +describe('ControllerMessenger', () => { + afterEach(() => { + sinon.restore(); + }); + + it('should allow registering and calling an action handler', () => { + type CountAction = { type: 'count'; handler: (increment: number) => void }; + const controllerMessenger = new ControllerMessenger(); + + let count = 0; + controllerMessenger.registerActionHandler('count', (increment: number) => { + count += increment; + }); + controllerMessenger.call('count', 1); + + expect(count).toEqual(1); + }); + + it('should allow registering and calling multiple different action handlers', () => { + type MessageAction = + | { type: 'concat'; handler: (message: string) => void } + | { type: 'reset'; handler: (initialMessage: string) => void }; + const controllerMessenger = new ControllerMessenger(); + + let message = ''; + controllerMessenger.registerActionHandler('reset', (initialMessage: string) => { + message = initialMessage; + }); + controllerMessenger.registerActionHandler('concat', (s: string) => { + message += s; + }); + + controllerMessenger.call('reset', 'hello'); + controllerMessenger.call('concat', ', world'); + + expect(message).toEqual('hello, world'); + }); + + it('should allow registering and calling an action handler with no parameters', () => { + type IncrementAction = { type: 'increment'; handler: () => void }; + const controllerMessenger = new ControllerMessenger(); + + let count = 0; + controllerMessenger.registerActionHandler('increment', () => { + count += 1; + }); + controllerMessenger.call('increment'); + + expect(count).toEqual(1); + }); + + it('should allow registering and calling an action handler with multiple parameters', () => { + type MessageAction = { type: 'message'; handler: (to: string, message: string) => void }; + const controllerMessenger = new ControllerMessenger(); + + const messages: Record = {}; + controllerMessenger.registerActionHandler('message', (to, message) => { + messages[to] = message; + }); + controllerMessenger.call('message', '0x123', 'hello'); + + expect(messages['0x123']).toEqual('hello'); + }); + + it('should allow registering and calling an action handler with a return value', () => { + type AddAction = { type: 'add'; handler: (a: number, b: number) => number }; + const controllerMessenger = new ControllerMessenger(); + + controllerMessenger.registerActionHandler('add', (a, b) => { + return a + b; + }); + const result = controllerMessenger.call('add', 5, 10); + + expect(result).toEqual(15); + }); + + it('should not allow registering multiple action handlers under the same name', () => { + type PingAction = { type: 'ping'; handler: () => void }; + const controllerMessenger = new ControllerMessenger(); + + controllerMessenger.registerActionHandler('ping', () => undefined); + + expect(() => { + controllerMessenger.registerActionHandler('ping', () => undefined); + }).toThrow(); + }); + + it('should throw when calling unregistered action', () => { + type PingAction = { type: 'ping'; handler: () => void }; + const controllerMessenger = new ControllerMessenger(); + + expect(() => { + controllerMessenger.call('ping'); + }).toThrow(); + }); + + it('should throw when calling an action that has been unregistered', () => { + type PingAction = { type: 'ping'; handler: () => void }; + const controllerMessenger = new ControllerMessenger(); + + expect(() => { + controllerMessenger.call('ping'); + }).toThrow(); + + let pingCount = 0; + controllerMessenger.registerActionHandler('ping', () => { + pingCount += 1; + }); + + controllerMessenger.unregisterActionHandler('ping'); + + expect(() => { + controllerMessenger.call('ping'); + }).toThrow(); + expect(pingCount).toEqual(0); + }); + + it('should throw when calling an action after actions have been reset', () => { + type PingAction = { type: 'ping'; handler: () => void }; + const controllerMessenger = new ControllerMessenger(); + + expect(() => { + controllerMessenger.call('ping'); + }).toThrow(); + + let pingCount = 0; + controllerMessenger.registerActionHandler('ping', () => { + pingCount += 1; + }); + + controllerMessenger.clearActions(); + + expect(() => { + controllerMessenger.call('ping'); + }).toThrow(); + expect(pingCount).toEqual(0); + }); + + it('should publish event to subscriber', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + controllerMessenger.subscribe('message', handler); + controllerMessenger.publish('message', 'hello'); + + expect(handler.calledWithExactly('hello')).toBeTruthy(); + expect(handler.callCount).toEqual(1); + }); + + it('should allow publishing multiple different events to subscriber', () => { + type MessageEvent = { type: 'message'; payload: [string] } | { type: 'ping'; payload: [] }; + const controllerMessenger = new ControllerMessenger(); + + const messageHandler = sinon.stub(); + const pingHandler = sinon.stub(); + controllerMessenger.subscribe('message', messageHandler); + controllerMessenger.subscribe('ping', pingHandler); + + controllerMessenger.publish('message', 'hello'); + controllerMessenger.publish('ping'); + + expect(messageHandler.calledWithExactly('hello')).toBeTruthy(); + expect(messageHandler.callCount).toEqual(1); + expect(pingHandler.calledWithExactly()).toBeTruthy(); + expect(pingHandler.callCount).toEqual(1); + }); + + it('should publish event with no payload to subscriber', () => { + type PingEvent = { type: 'ping'; payload: [] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + controllerMessenger.subscribe('ping', handler); + controllerMessenger.publish('ping'); + + expect(handler.calledWithExactly()).toBeTruthy(); + expect(handler.callCount).toEqual(1); + }); + + it('should publish event with multiple payload parameters to subscriber', () => { + type MessageEvent = { type: 'message'; payload: [string, string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + controllerMessenger.subscribe('message', handler); + controllerMessenger.publish('message', 'hello', 'there'); + + expect(handler.calledWithExactly('hello', 'there')).toBeTruthy(); + expect(handler.callCount).toEqual(1); + }); + + it('should publish event once to subscriber even if subscribed multiple times', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + controllerMessenger.subscribe('message', handler); + controllerMessenger.subscribe('message', handler); + controllerMessenger.publish('message', 'hello'); + + expect(handler.calledWithExactly('hello')).toBeTruthy(); + expect(handler.callCount).toEqual(1); + }); + + it('should publish event to many subscribers', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler1 = sinon.stub(); + const handler2 = sinon.stub(); + controllerMessenger.subscribe('message', handler1); + controllerMessenger.subscribe('message', handler2); + controllerMessenger.publish('message', 'hello'); + + expect(handler1.calledWithExactly('hello')).toBeTruthy(); + expect(handler1.callCount).toEqual(1); + expect(handler2.calledWithExactly('hello')).toBeTruthy(); + expect(handler2.callCount).toEqual(1); + }); + + it('should not call subscriber after unsubscribing', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + controllerMessenger.subscribe('message', handler); + controllerMessenger.unsubscribe('message', handler); + controllerMessenger.publish('message', 'hello'); + + expect(handler.callCount).toEqual(0); + }); + + it('should throw when unsubscribing when there are no subscriptions', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + expect(() => controllerMessenger.unsubscribe('message', handler)).toThrow(); + }); + + it('should throw when unsubscribing a handler that is not subscribed', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler1 = sinon.stub(); + const handler2 = sinon.stub(); + controllerMessenger.subscribe('message', handler1); + + expect(() => controllerMessenger.unsubscribe('message', handler2)).toThrow(); + }); + + it('should not call subscriber after clearing event subscriptions', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + controllerMessenger.subscribe('message', handler); + controllerMessenger.clearEventSubscriptions('message'); + controllerMessenger.publish('message', 'hello'); + + expect(handler.callCount).toEqual(0); + }); + + it('should not throw when clearing event that has no subscriptions', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + expect(() => controllerMessenger.clearEventSubscriptions('message')).not.toThrow(); + }); + + it('should not call subscriber after resetting subscriptions', () => { + type MessageEvent = { type: 'message'; payload: [string] }; + const controllerMessenger = new ControllerMessenger(); + + const handler = sinon.stub(); + controllerMessenger.subscribe('message', handler); + controllerMessenger.clearSubscriptions(); + controllerMessenger.publish('message', 'hello'); + + expect(handler.callCount).toEqual(0); + }); +}); diff --git a/src/ControllerMessenger.ts b/src/ControllerMessenger.ts new file mode 100644 index 00000000000..a70fc41f2f4 --- /dev/null +++ b/src/ControllerMessenger.ts @@ -0,0 +1,89 @@ +export type ActionHandler = ( + ...args: ExtractActionParameters +) => ExtractActionResponse; +export type ExtractActionParameters = Action extends { type: T; handler: (...args: infer H) => any } + ? H + : never; +export type ExtractActionResponse = Action extends { type: T; handler: (...args: any) => infer H } + ? H + : never; + +export type ExtractEvenHandler = Event extends { type: T; payload: infer P } + ? P extends any[] + ? (...payload: P) => void + : never + : never; +export type ExtractEventPayload = Event extends { type: T; payload: infer P } ? P : never; + +type ActionConstraint = { type: string; handler: (...args: any) => unknown }; +type EventConstraint = { type: string; payload: unknown[] }; + +export class ControllerMessenger { + private actions = new Map(); + + private events = new Map>(); + + registerActionHandler(action: T, handler: ActionHandler) { + if (this.actions.has(action)) { + throw new Error(`A handler for ${action} has already been registered`); + } + this.actions.set(action, handler); + } + + unregisterActionHandler(action: T) { + this.actions.delete(action); + } + + clearActions() { + this.actions.clear(); + } + + call( + action: T, + ...params: ExtractActionParameters + ): ExtractActionResponse { + const handler = this.actions.get(action) as ActionHandler; + if (!handler) { + throw new Error(`A handler for ${action} has not been registered`); + } + return handler(...params); + } + + publish(event: E, ...payload: ExtractEventPayload) { + const subscribers = this.events.get(event) as Set>; + + if (subscribers) { + for (const eventHandler of subscribers) { + eventHandler(...payload); + } + } + } + + subscribe(event: E, handler: ExtractEvenHandler) { + let subscribers = this.events.get(event); + if (!subscribers) { + subscribers = new Set(); + } + subscribers.add(handler); + this.events.set(event, subscribers); + } + + unsubscribe(event: E, handler: ExtractEvenHandler) { + const subscribers = this.events.get(event); + + if (!subscribers || !subscribers.has(handler)) { + throw new Error(`Subscription not found for event: '${event}'`); + } + + subscribers.delete(handler); + this.events.set(event, subscribers); + } + + clearEventSubscriptions(event: E) { + this.events.delete(event); + } + + clearSubscriptions() { + this.events.clear(); + } +} From 85fc3ef2422d82cab915979dcc0fc59ae6285ce4 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 8 Mar 2021 14:47:03 -0330 Subject: [PATCH 2/6] Remove `subscribe` and `unsubscribe` BaseController methods The base controller no longer exposes methods to subscribe and unsubscribe to state changes. Instead all subscriptions and unsubscriptions are done with the controller messaging system. --- src/BaseControllerV2.test.ts | 32 ++++++++++++++------------------ src/BaseControllerV2.ts | 18 ------------------ 2 files changed, 14 insertions(+), 36 deletions(-) diff --git a/src/BaseControllerV2.test.ts b/src/BaseControllerV2.test.ts index c5bd062e411..d03f8d3bcca 100644 --- a/src/BaseControllerV2.test.ts +++ b/src/BaseControllerV2.test.ts @@ -129,8 +129,8 @@ describe('BaseController', () => { const listener1 = sinon.stub(); const listener2 = sinon.stub(); - controller.subscribe(listener1); - controller.subscribe(listener2); + controllerMessenger.subscribe('CountController:state-change', listener1); + controllerMessenger.subscribe('CountController:state-change', listener2); controller.update(() => { return { count: 1 }; }); @@ -151,8 +151,9 @@ describe('BaseController', () => { ); const listener1 = sinon.stub(); - controller.subscribe(listener1); - controller.subscribe(listener1); + controllerMessenger.subscribe('CountController:state-change', listener1); + controllerMessenger.subscribe('CountController:state-change', listener1); + controller.update(() => { return { count: 1 }; }); @@ -171,8 +172,8 @@ describe('BaseController', () => { ); const listener1 = sinon.stub(); - controller.subscribe(listener1); - controller.unsubscribe(listener1); + controllerMessenger.subscribe('CountController:state-change', listener1); + controllerMessenger.unsubscribe('CountController:state-change', listener1); controller.update(() => { return { count: 1 }; }); @@ -190,9 +191,9 @@ describe('BaseController', () => { ); const listener1 = sinon.stub(); - controller.subscribe(listener1); - controller.subscribe(listener1); - controller.unsubscribe(listener1); + controllerMessenger.subscribe('CountController:state-change', listener1); + controllerMessenger.subscribe('CountController:state-change', listener1); + controllerMessenger.unsubscribe('CountController:state-change', listener1); controller.update(() => { return { count: 1 }; }); @@ -202,16 +203,11 @@ describe('BaseController', () => { it('should throw when unsubscribing listener who was never subscribed', () => { const controllerMessenger = new ControllerMessenger(); - const controller = new MockController( - controllerMessenger, - 'CountController', - { count: 0 }, - CountControllerStateMetadata, - ); + new MockController(controllerMessenger, 'CountController', { count: 0 }, CountControllerStateMetadata); const listener1 = sinon.stub(); expect(() => { - controller.unsubscribe(listener1); + controllerMessenger.unsubscribe('CountController:state-change', listener1); }).toThrow(); }); @@ -226,8 +222,8 @@ describe('BaseController', () => { const listener1 = sinon.stub(); const listener2 = sinon.stub(); - controller.subscribe(listener1); - controller.subscribe(listener2); + controllerMessenger.subscribe('CountController:state-change', listener1); + controllerMessenger.subscribe('CountController:state-change', listener2); controller.destroy(); controller.update(() => { return { count: 1 }; diff --git a/src/BaseControllerV2.ts b/src/BaseControllerV2.ts index 2365b431445..030497f828a 100644 --- a/src/BaseControllerV2.ts +++ b/src/BaseControllerV2.ts @@ -140,24 +140,6 @@ export class BaseController> throw new Error(`Controller state cannot be directly mutated; use 'update' method instead.`); } - /** - * Adds new listener to be notified of state changes - * - * @param listener - Callback triggered when state changes - */ - subscribe(listener: Listener) { - this.messagingSystem.subscribe(`${this.name}:state-change` as `${N}:state-change`, listener); - } - - /** - * Removes existing listener from receiving state changes - * - * @param listener - Callback to remove - */ - unsubscribe(listener: Listener) { - this.messagingSystem.unsubscribe(`${this.name}:state-change` as `${N}:state-change`, listener); - } - /** * Updates controller state. Accepts a callback that is passed a draft copy * of the controller state. If a value is returned, it is set as the new From ed6488f389f67a2b890289135cc1d0abbed10719 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 8 Mar 2021 14:49:47 -0330 Subject: [PATCH 3/6] Rename `state-change` event to `stateChange` This event name matches the naming convention of the event prefix, which is also camelCase. --- src/BaseControllerV2.test.ts | 26 +++++++++++++------------- src/BaseControllerV2.ts | 8 ++++---- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/BaseControllerV2.test.ts b/src/BaseControllerV2.test.ts index d03f8d3bcca..ab9b777f606 100644 --- a/src/BaseControllerV2.test.ts +++ b/src/BaseControllerV2.test.ts @@ -9,7 +9,7 @@ type CountControllerState = { }; type CountControllerEvent = { - type: `CountController:state-change`; + type: `CountController:stateChange`; payload: [CountControllerState, Patch[]]; }; @@ -129,8 +129,8 @@ describe('BaseController', () => { const listener1 = sinon.stub(); const listener2 = sinon.stub(); - controllerMessenger.subscribe('CountController:state-change', listener1); - controllerMessenger.subscribe('CountController:state-change', listener2); + controllerMessenger.subscribe('CountController:stateChange', listener1); + controllerMessenger.subscribe('CountController:stateChange', listener2); controller.update(() => { return { count: 1 }; }); @@ -151,8 +151,8 @@ describe('BaseController', () => { ); const listener1 = sinon.stub(); - controllerMessenger.subscribe('CountController:state-change', listener1); - controllerMessenger.subscribe('CountController:state-change', listener1); + controllerMessenger.subscribe('CountController:stateChange', listener1); + controllerMessenger.subscribe('CountController:stateChange', listener1); controller.update(() => { return { count: 1 }; @@ -172,8 +172,8 @@ describe('BaseController', () => { ); const listener1 = sinon.stub(); - controllerMessenger.subscribe('CountController:state-change', listener1); - controllerMessenger.unsubscribe('CountController:state-change', listener1); + controllerMessenger.subscribe('CountController:stateChange', listener1); + controllerMessenger.unsubscribe('CountController:stateChange', listener1); controller.update(() => { return { count: 1 }; }); @@ -191,9 +191,9 @@ describe('BaseController', () => { ); const listener1 = sinon.stub(); - controllerMessenger.subscribe('CountController:state-change', listener1); - controllerMessenger.subscribe('CountController:state-change', listener1); - controllerMessenger.unsubscribe('CountController:state-change', listener1); + controllerMessenger.subscribe('CountController:stateChange', listener1); + controllerMessenger.subscribe('CountController:stateChange', listener1); + controllerMessenger.unsubscribe('CountController:stateChange', listener1); controller.update(() => { return { count: 1 }; }); @@ -207,7 +207,7 @@ describe('BaseController', () => { const listener1 = sinon.stub(); expect(() => { - controllerMessenger.unsubscribe('CountController:state-change', listener1); + controllerMessenger.unsubscribe('CountController:stateChange', listener1); }).toThrow(); }); @@ -222,8 +222,8 @@ describe('BaseController', () => { const listener1 = sinon.stub(); const listener2 = sinon.stub(); - controllerMessenger.subscribe('CountController:state-change', listener1); - controllerMessenger.subscribe('CountController:state-change', listener2); + controllerMessenger.subscribe('CountController:stateChange', listener1); + controllerMessenger.subscribe('CountController:stateChange', listener2); controller.destroy(); controller.update(() => { return { count: 1 }; diff --git a/src/BaseControllerV2.ts b/src/BaseControllerV2.ts index 030497f828a..bbc01112b62 100644 --- a/src/BaseControllerV2.ts +++ b/src/BaseControllerV2.ts @@ -101,7 +101,7 @@ type Json = null | boolean | number | string | Json[] | { [prop: string]: Json } export class BaseController> { private internalState: IsJsonable; - private messagingSystem: ControllerMessenger; + private messagingSystem: ControllerMessenger; private name: N; @@ -116,7 +116,7 @@ export class BaseController> * and which parts should be persisted. */ constructor( - messagingSystem: ControllerMessenger, + messagingSystem: ControllerMessenger, name: N, state: IsJsonable, metadata: StateMetadata, @@ -152,7 +152,7 @@ export class BaseController> protected update(callback: (state: Draft>) => void | IsJsonable) { const [nextState, patches] = produceWithPatches(this.internalState, callback); this.internalState = nextState as IsJsonable; - this.messagingSystem.publish(`${this.name}:state-change` as `${N}:state-change`, nextState as S, patches); + this.messagingSystem.publish(`${this.name}:stateChange` as `${N}:stateChange`, nextState as S, patches); } /** @@ -165,7 +165,7 @@ export class BaseController> * listeners from being garbage collected. */ protected destroy() { - this.messagingSystem.clearEventSubscriptions(`${this.name}:state-change` as `${N}:state-change`); + this.messagingSystem.clearEventSubscriptions(`${this.name}:stateChange` as `${N}:stateChange`); } } From cc8f81debd79231671978147515236dbff46b710 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Mon, 8 Mar 2021 17:53:58 -0330 Subject: [PATCH 4/6] Add JSDoc comments, and rename type parameters JSDoc comments have been added for each method on the controller messenger instance. Additionally, the `event` parameter has been renamed to `eventType`, and the `action` parameter has been renamed to `actionType`. This was done while adding docs because it made the documentation easier to understand. --- src/ControllerMessenger.ts | 112 ++++++++++++++++++++++++++++++------- 1 file changed, 92 insertions(+), 20 deletions(-) diff --git a/src/ControllerMessenger.ts b/src/ControllerMessenger.ts index a70fc41f2f4..e0face32768 100644 --- a/src/ControllerMessenger.ts +++ b/src/ControllerMessenger.ts @@ -23,34 +23,76 @@ export class ControllerMessenger>(); - registerActionHandler(action: T, handler: ActionHandler) { - if (this.actions.has(action)) { - throw new Error(`A handler for ${action} has already been registered`); + /** + * Register an action handler. + * + * This will make the registered function available to call via the `call` method. + * + * @param actionType - The action type. This is a unqiue identifier for this action. + * @param handler- The action handler. This function gets called when the `call` method is + * invoked with the given action type. + * @throws Will throw when a handler has been registered for this action type already. + */ + registerActionHandler(actionType: T, handler: ActionHandler) { + if (this.actions.has(actionType)) { + throw new Error(`A handler for ${actionType} has already been registered`); } - this.actions.set(action, handler); + this.actions.set(actionType, handler); } - unregisterActionHandler(action: T) { - this.actions.delete(action); + /** + * Unregister an action handler. + * + * This will prevent this action from being called. + * + * @param actionType - The action type. This is a unqiue identifier for this action. + */ + unregisterActionHandler(actionType: T) { + this.actions.delete(actionType); } + /** + * Unregister all action handlers. + * + * This prevents all actions from being called. + */ clearActions() { this.actions.clear(); } + /** + * Call an action. + * + * This function will call the action handler corresponding to the given action type, passing + * along any parameters given. + * + * @param actionType - The action type. This is a unqiue identifier for this action. + * @param params - The action parameters. These must match the type of the parameters of the + * registered action handler. + * @throws Will throw when no handler has been registered for the given type. + */ call( - action: T, + actionType: T, ...params: ExtractActionParameters ): ExtractActionResponse { - const handler = this.actions.get(action) as ActionHandler; + const handler = this.actions.get(actionType) as ActionHandler; if (!handler) { - throw new Error(`A handler for ${action} has not been registered`); + throw new Error(`A handler for ${actionType} has not been registered`); } return handler(...params); } - publish(event: E, ...payload: ExtractEventPayload) { - const subscribers = this.events.get(event) as Set>; + /** + * Publish an event. + * + * Publishes the given payload to all subscribers of the given event type. + * + * @param eventType - The event type. This is a unique identifier for this event. + * @param payload - The event payload. The type of the parameters for each event handler must + * match the type of this payload. + */ + publish(eventType: E, ...payload: ExtractEventPayload) { + const subscribers = this.events.get(eventType) as Set>; if (subscribers) { for (const eventHandler of subscribers) { @@ -59,30 +101,60 @@ export class ControllerMessenger(event: E, handler: ExtractEvenHandler) { - let subscribers = this.events.get(event); + /** + * Subscribe to an event. + * + * Registers the given function as an event handler for the given event type. + * + * @param eventType - The event type. This is a unique identifier for this event. + * @param handler - The event handler. The type of the parameters for this event handler must + * match the type of the payload for this event type. + */ + subscribe(eventType: E, handler: ExtractEvenHandler) { + let subscribers = this.events.get(eventType); if (!subscribers) { subscribers = new Set(); } subscribers.add(handler); - this.events.set(event, subscribers); + this.events.set(eventType, subscribers); } - unsubscribe(event: E, handler: ExtractEvenHandler) { - const subscribers = this.events.get(event); + /** + * Unsubscribe from an event. + * + * Unregisters the given function as an event handler for the given event. + * + * @param eventType - The event type. This is a unique identifier for this event. + * @param handler - The event handler to unregister. + * @throws Will throw when the given event handler is not registered for this event. + */ + unsubscribe(eventType: E, handler: ExtractEvenHandler) { + const subscribers = this.events.get(eventType); if (!subscribers || !subscribers.has(handler)) { - throw new Error(`Subscription not found for event: '${event}'`); + throw new Error(`Subscription not found for event: '${eventType}'`); } subscribers.delete(handler); - this.events.set(event, subscribers); + this.events.set(eventType, subscribers); } - clearEventSubscriptions(event: E) { - this.events.delete(event); + /** + * Clear subscriptions for a specific event. + * + * This will remove all subscribed handlers for this event. + * + * @param eventType - The event type. This is a unique identifier for this event. + */ + clearEventSubscriptions(eventType: E) { + this.events.delete(eventType); } + /** + * Clear all subscriptions. + * + * This will remove all subscribed handlers for all events. + */ clearSubscriptions() { this.events.clear(); } From 3ed24a62d19768258ca9925329e1bd84fe1172ea Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 9 Mar 2021 01:37:50 -0330 Subject: [PATCH 5/6] Add JSDOc comment for ControllerMessenger class --- src/ControllerMessenger.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/ControllerMessenger.ts b/src/ControllerMessenger.ts index e0face32768..4010494d174 100644 --- a/src/ControllerMessenger.ts +++ b/src/ControllerMessenger.ts @@ -18,6 +18,13 @@ export type ExtractEventPayload = Event extends { type: T; payload: in type ActionConstraint = { type: string; handler: (...args: any) => unknown }; type EventConstraint = { type: string; payload: unknown[] }; +/** + * A messaging system for controllers. + * + * The controller messenger allows registering functions as 'actions' that can be called elsewhere, + * and it allows publishing and subscribing to events. Both actions and events are identified by + * unique strings. + */ export class ControllerMessenger { private actions = new Map(); From 1671284c5b9f5eaea3232e7b1c33a26d5cd1f968 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 9 Mar 2021 01:51:04 -0330 Subject: [PATCH 6/6] Remove unused type exports These types were only used internally. We can export them again if and when that becomes useful. --- src/ControllerMessenger.ts | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/ControllerMessenger.ts b/src/ControllerMessenger.ts index 4010494d174..12568efc6c1 100644 --- a/src/ControllerMessenger.ts +++ b/src/ControllerMessenger.ts @@ -1,19 +1,15 @@ -export type ActionHandler = ( +type ActionHandler = ( ...args: ExtractActionParameters ) => ExtractActionResponse; -export type ExtractActionParameters = Action extends { type: T; handler: (...args: infer H) => any } - ? H - : never; -export type ExtractActionResponse = Action extends { type: T; handler: (...args: any) => infer H } - ? H - : never; +type ExtractActionParameters = Action extends { type: T; handler: (...args: infer H) => any } ? H : never; +type ExtractActionResponse = Action extends { type: T; handler: (...args: any) => infer H } ? H : never; -export type ExtractEvenHandler = Event extends { type: T; payload: infer P } +type ExtractEvenHandler = Event extends { type: T; payload: infer P } ? P extends any[] ? (...payload: P) => void : never : never; -export type ExtractEventPayload = Event extends { type: T; payload: infer P } ? P : never; +type ExtractEventPayload = Event extends { type: T; payload: infer P } ? P : never; type ActionConstraint = { type: string; handler: (...args: any) => unknown }; type EventConstraint = { type: string; payload: unknown[] };