-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore: improved LDEmitter #207
chore: improved LDEmitter #207
Conversation
yusinto
commented
Jul 18, 2023
- Improved emitter design
- Improved jest config
- Added unit tests
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.
Renamed to jest.config.json to be consistent with other projects in the repo.
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 like maybeXXX
. Secondly I don't understand why maybe reporting an error is the responsibility of an Emitter.
export default class LDEmitter { | ||
private et: EventTarget = new EventTarget(); |
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 choose not to extend because we are designing this to follow EventEmitter's interface. Further, for react-native, we may need to refactor EventTarget
to be a generic so we can swap it out with a polyfill. For example:
// 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 comment
The 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 comment
The 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 comment
The 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.
f539c7a
into
yus/sc-208024/scaffold-js-common-in-js-core