-
Notifications
You must be signed in to change notification settings - Fork 27
chore: improved LDEmitter #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
import '@testing-library/jest-dom'; |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
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(); | ||
expect(emitter.listenerCount('error')).toEqual(0); | ||
}); | ||
|
||
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); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
export type EventName = 'change' | 'internal-change' | 'ready' | 'initialized' | 'failed' | 'error'; | ||
|
||
/** | ||
* 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 LDEmitter { | ||
private et: EventTarget = new EventTarget(); | ||
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I choose not to extend because we are designing this to follow EventEmitter's interface. Further, for react-native, we may need to refactor // react-native will pass in a polyfill.
// browsers will use the built-in api.
export default class LDEmitter<IEventTarget> { There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we just following event emitters interface for backward compatibility, or ease of porting, or both, other? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More for ease of porting. However, I want to know your thoughts. Please expound. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it makes porting easier, then I am fine with it. I just wanted to make sure there wasn't some technical reason we specifically wanted to keep that interface. |
||
private listeners: Map<EventName, EventListener[]> = new Map(); | ||
|
||
/** | ||
* Cache all listeners in a Map so we can remove them later | ||
* @param name string event name | ||
* @param listener function to handle the event | ||
* @private | ||
*/ | ||
private saveListener(name: EventName, listener: EventListener) { | ||
if (!this.listeners.has(name)) { | ||
this.listeners.set(name, [listener]); | ||
} else { | ||
this.listeners.get(name)?.push(listener); | ||
} | ||
} | ||
|
||
on(name: EventName, listener: Function) { | ||
const customListener = (e: Event) => { | ||
const { detail } = e as CustomEvent; | ||
|
||
// invoke listener with args from CustomEvent | ||
listener(...detail); | ||
}; | ||
this.saveListener(name, customListener); | ||
this.et.addEventListener(name, customListener); | ||
} | ||
|
||
off(name: EventName) { | ||
this.listeners.get(name)?.forEach((l) => { | ||
this.et.removeEventListener(name, l); | ||
}); | ||
this.listeners.delete(name); | ||
} | ||
|
||
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; | ||
} | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm omitting
maybeReportError
from the legacy api for now. I don't understand why anyone will trust a method likemaybeXXX
. Secondly I don't understand why maybe reporting an error is the responsibility of an Emitter.