From 627efe6aab62961d6b0ae8419f0ffa4aae7ce439 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Mon, 17 Jul 2023 10:02:21 -0700 Subject: [PATCH 1/3] chore: use private instance of EventTarget instead of extending --- .../shared/sdk-client/src/api/LDEventTarget.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/shared/sdk-client/src/api/LDEventTarget.ts b/packages/shared/sdk-client/src/api/LDEventTarget.ts index 95a87f9403..913b6c84f6 100644 --- a/packages/shared/sdk-client/src/api/LDEventTarget.ts +++ b/packages/shared/sdk-client/src/api/LDEventTarget.ts @@ -8,7 +8,8 @@ export type EventName = 'change' | 'internal-change' | 'ready' | 'initialized' | * because the react-native repo uses it too. * https://github.com/mysticatea/event-target-shim */ -export default class LDEventTarget extends EventTarget { +export default class LDEventEmitter { + private et: EventTarget = new EventTarget(); private listeners: Map = new Map(); /** @@ -27,22 +28,18 @@ export default class LDEventTarget extends EventTarget { public on(name: EventName, listener: Function) { const customListener = (e: Event) => { - const { detail } = e as CustomEvent; - // invoke listener with additional args from CustomEvent.detail - listener(...listener.arguments, ...detail); + listener(...listener.arguments, ...(e as CustomEvent).detail); }; this.saveListener(name, customListener); - - super.addEventListener(name, customListener); + this.et.addEventListener(name, customListener); } public off(name: EventName) { - this.listeners.get(name)?.forEach((l) => super.removeEventListener(name, l)); + this.listeners.get(name)?.forEach((l) => this.et.removeEventListener(name, l)); } - public emit(name: EventName, ...detail: any[]): boolean { - const c = new CustomEvent(name, { detail }); - return super.dispatchEvent(c); + public emit(name: EventName, ...detail: any[]) { + this.et.dispatchEvent(new CustomEvent(name, { detail })); } } From abdd532f77f5d0b006732ad4dffdaec70ae6da20 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Mon, 17 Jul 2023 17:11:02 -0700 Subject: [PATCH 2/3] chore: improved event emitter. configured jest and added unit tests. --- .../sdk-client/jest-setupFilesAfterEnv.ts | 1 + packages/shared/sdk-client/jest.config.js | 7 -- packages/shared/sdk-client/jest.config.json | 10 +++ packages/shared/sdk-client/package.json | 27 ++++--- .../sdk-client/src/api/LDEmitter.test.ts | 76 +++++++++++++++++++ .../api/{LDEventTarget.ts => LDEmitter.ts} | 30 +++++--- packages/shared/sdk-client/tsconfig.json | 1 + 7 files changed, 123 insertions(+), 29 deletions(-) create mode 100644 packages/shared/sdk-client/jest-setupFilesAfterEnv.ts delete mode 100644 packages/shared/sdk-client/jest.config.js create mode 100644 packages/shared/sdk-client/jest.config.json create mode 100644 packages/shared/sdk-client/src/api/LDEmitter.test.ts rename packages/shared/sdk-client/src/api/{LDEventTarget.ts => LDEmitter.ts} (62%) diff --git a/packages/shared/sdk-client/jest-setupFilesAfterEnv.ts b/packages/shared/sdk-client/jest-setupFilesAfterEnv.ts new file mode 100644 index 0000000000..7b0828bfa8 --- /dev/null +++ b/packages/shared/sdk-client/jest-setupFilesAfterEnv.ts @@ -0,0 +1 @@ +import '@testing-library/jest-dom'; diff --git a/packages/shared/sdk-client/jest.config.js b/packages/shared/sdk-client/jest.config.js deleted file mode 100644 index f106eb3bc9..0000000000 --- a/packages/shared/sdk-client/jest.config.js +++ /dev/null @@ -1,7 +0,0 @@ -module.exports = { - transform: { '^.+\\.ts?$': 'ts-jest' }, - testMatch: ['**/__tests__/**/*test.ts?(x)'], - testEnvironment: 'node', - moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json', 'node'], - collectCoverageFrom: ['src/**/*.ts'], -}; diff --git a/packages/shared/sdk-client/jest.config.json b/packages/shared/sdk-client/jest.config.json new file mode 100644 index 0000000000..65ddc27dd0 --- /dev/null +++ b/packages/shared/sdk-client/jest.config.json @@ -0,0 +1,10 @@ +{ + "transform": { "^.+\\.ts?$": "ts-jest" }, + "testMatch": ["**/*.test.ts?(x)"], + "testPathIgnorePatterns": ["node_modules", "example", "dist"], + "modulePathIgnorePatterns": ["dist"], + "testEnvironment": "jsdom", + "moduleFileExtensions": ["ts", "tsx", "js", "jsx", "json", "node"], + "collectCoverageFrom": ["src/**/*.ts"], + "setupFilesAfterEnv": ["./jest-setupFilesAfterEnv.ts"] +} diff --git a/packages/shared/sdk-client/package.json b/packages/shared/sdk-client/package.json index abc87d06a1..c923e095df 100644 --- a/packages/shared/sdk-client/package.json +++ b/packages/shared/sdk-client/package.json @@ -30,25 +30,28 @@ "//": "Pinned semver because 7.5.0 introduced require('util') without the node: prefix", "dependencies": { "@launchdarkly/js-sdk-common": "1.0.2", - "semver": "7.4.0" + "semver": "7.5.4" }, "devDependencies": { - "@types/jest": "^29.5.2", + "@testing-library/dom": "^9.3.1", + "@testing-library/jest-dom": "^5.16.5", + "@types/jest": "^29.5.3", "@types/semver": "^7.5.0", - "@typescript-eslint/eslint-plugin": "^5.60.1", - "@typescript-eslint/parser": "^5.60.1", - "eslint": "^8.43.0", + "@typescript-eslint/eslint-plugin": "^6.1.0", + "@typescript-eslint/parser": "^6.1.0", + "eslint": "^8.45.0", "eslint-config-airbnb-base": "^15.0.0", - "eslint-config-airbnb-typescript": "^17.0.0", + "eslint-config-airbnb-typescript": "^17.1.0", "eslint-config-prettier": "^8.8.0", "eslint-plugin-import": "^2.27.5", - "eslint-plugin-prettier": "^4.2.1", - "jest": "^29.5.0", - "jest-diff": "^29.5.0", + "eslint-plugin-prettier": "^5.0.0", + "jest": "^29.6.1", + "jest-diff": "^29.6.1", + "jest-environment-jsdom": "^29.6.1", "launchdarkly-js-test-helpers": "^2.2.0", - "prettier": "^2.8.8", - "ts-jest": "^29.1.0", + "prettier": "^3.0.0", + "ts-jest": "^29.1.1", "typedoc": "0.23.26", - "typescript": "^4.6.3" + "typescript": "^5.1.6" } } diff --git a/packages/shared/sdk-client/src/api/LDEmitter.test.ts b/packages/shared/sdk-client/src/api/LDEmitter.test.ts new file mode 100644 index 0000000000..c946c1aa94 --- /dev/null +++ b/packages/shared/sdk-client/src/api/LDEmitter.test.ts @@ -0,0 +1,76 @@ +import LDEmitter from './LDEmitter'; + +describe('LDEmitter', () => { + const error = { type: 'network', message: 'unreachable' }; + let emitter: LDEmitter; + + beforeEach(() => { + jest.resetAllMocks(); + emitter = new LDEmitter(); + }); + + test('subscribe and handle', () => { + const errorHandler1 = jest.fn(); + const errorHandler2 = jest.fn(); + + emitter.on('error', errorHandler1); + emitter.on('error', errorHandler2); + emitter.emit('error', error); + + expect(errorHandler1).toHaveBeenCalledWith(error); + expect(errorHandler2).toHaveBeenCalledWith(error); + }); + + test('unsubscribe and handle', () => { + const errorHandler1 = jest.fn(); + const errorHandler2 = jest.fn(); + + emitter.on('error', errorHandler1); + emitter.on('error', errorHandler2); + emitter.off('error'); + emitter.emit('error', error); + + expect(errorHandler1).not.toHaveBeenCalled(); + expect(errorHandler2).not.toHaveBeenCalled(); + }); + + test('unsubscribing an event should not affect other events', () => { + const errorHandler = jest.fn(); + const changeHandler = jest.fn(); + + emitter.on('error', errorHandler); + emitter.on('change', changeHandler); + emitter.off('error'); // unsubscribe error handler + emitter.emit('error', error); + emitter.emit('change'); + + // change handler should still be affective + expect(changeHandler).toHaveBeenCalled(); + expect(errorHandler).not.toHaveBeenCalled(); + }); + + test('eventNames', () => { + const errorHandler1 = jest.fn(); + const changeHandler = jest.fn(); + const readyHandler = jest.fn(); + + emitter.on('error', errorHandler1); + emitter.on('change', changeHandler); + emitter.on('ready', readyHandler); + + expect(emitter.eventNames()).toEqual(['error', 'change', 'ready']); + }); + + test('listener count', () => { + const errorHandler1 = jest.fn(); + const errorHandler2 = jest.fn(); + const changeHandler = jest.fn(); + + emitter.on('error', errorHandler1); + emitter.on('error', errorHandler2); + emitter.on('change', changeHandler); + + expect(emitter.listenerCount('error')).toEqual(2); + expect(emitter.listenerCount('change')).toEqual(1); + }); +}); diff --git a/packages/shared/sdk-client/src/api/LDEventTarget.ts b/packages/shared/sdk-client/src/api/LDEmitter.ts similarity index 62% rename from packages/shared/sdk-client/src/api/LDEventTarget.ts rename to packages/shared/sdk-client/src/api/LDEmitter.ts index 913b6c84f6..5488f973b3 100644 --- a/packages/shared/sdk-client/src/api/LDEventTarget.ts +++ b/packages/shared/sdk-client/src/api/LDEmitter.ts @@ -1,21 +1,21 @@ -export type EventName = 'change' | 'internal-change' | 'ready' | 'initialized' | 'failed'; +export type EventName = 'change' | 'internal-change' | 'ready' | 'initialized' | 'failed' | 'error'; /** - * Needs WebApi EventTarget. + * This is an event emitter using the standard built-in EventTarget web api. * https://developer.mozilla.org/en-US/docs/Web/API/EventTarget * * In react-native use event-target-shim to polyfill EventTarget. This is safe * because the react-native repo uses it too. * https://github.com/mysticatea/event-target-shim */ -export default class LDEventEmitter { +export default class LDEmitter { private et: EventTarget = new EventTarget(); private listeners: Map = new Map(); /** * Cache all listeners in a Map so we can remove them later - * @param e EventName - * @param listener The event handler + * @param name string event name + * @param listener function to handle the event * @private */ private saveListener(name: EventName, listener: EventListener) { @@ -26,20 +26,30 @@ export default class LDEventEmitter { } } - public on(name: EventName, listener: Function) { + on(name: EventName, listener: Function) { const customListener = (e: Event) => { - // invoke listener with additional args from CustomEvent.detail - listener(...listener.arguments, ...(e as CustomEvent).detail); + const { detail } = e as CustomEvent; + + // invoke listener with args from CustomEvent + listener(...detail); }; this.saveListener(name, customListener); this.et.addEventListener(name, customListener); } - public off(name: EventName) { + off(name: EventName) { this.listeners.get(name)?.forEach((l) => this.et.removeEventListener(name, l)); } - public emit(name: EventName, ...detail: any[]) { + emit(name: EventName, ...detail: any[]) { this.et.dispatchEvent(new CustomEvent(name, { detail })); } + + eventNames(): string[] { + return [...this.listeners.keys()]; + } + + listenerCount(name: EventName): number { + return this.listeners.get(name)?.length ?? 0; + } } diff --git a/packages/shared/sdk-client/tsconfig.json b/packages/shared/sdk-client/tsconfig.json index b1088a2d8c..dd1ef125d0 100644 --- a/packages/shared/sdk-client/tsconfig.json +++ b/packages/shared/sdk-client/tsconfig.json @@ -14,5 +14,6 @@ "declarationMap": true, // enables importers to jump to source "stripInternal": true }, + "include": ["src", "./jest-setupFilesAfterEnv.ts"], "exclude": ["**/*.test.ts", "dist", "node_modules", "__tests__"] } From 9af0976afd9ad95b2eb5472b3ce7f61ae57b3bc0 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 18 Jul 2023 14:57:34 -0700 Subject: [PATCH 3/3] fix: remove internal listeners cache --- packages/shared/sdk-client/src/api/LDEmitter.test.ts | 1 + packages/shared/sdk-client/src/api/LDEmitter.ts | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/shared/sdk-client/src/api/LDEmitter.test.ts b/packages/shared/sdk-client/src/api/LDEmitter.test.ts index c946c1aa94..cba0d4bcd2 100644 --- a/packages/shared/sdk-client/src/api/LDEmitter.test.ts +++ b/packages/shared/sdk-client/src/api/LDEmitter.test.ts @@ -32,6 +32,7 @@ describe('LDEmitter', () => { expect(errorHandler1).not.toHaveBeenCalled(); expect(errorHandler2).not.toHaveBeenCalled(); + expect(emitter.listenerCount('error')).toEqual(0); }); test('unsubscribing an event should not affect other events', () => { diff --git a/packages/shared/sdk-client/src/api/LDEmitter.ts b/packages/shared/sdk-client/src/api/LDEmitter.ts index 5488f973b3..5d587f874a 100644 --- a/packages/shared/sdk-client/src/api/LDEmitter.ts +++ b/packages/shared/sdk-client/src/api/LDEmitter.ts @@ -38,7 +38,10 @@ export default class LDEmitter { } off(name: EventName) { - this.listeners.get(name)?.forEach((l) => this.et.removeEventListener(name, l)); + this.listeners.get(name)?.forEach((l) => { + this.et.removeEventListener(name, l); + }); + this.listeners.delete(name); } emit(name: EventName, ...detail: any[]) {