Core: Implement event emitter #1087
Conversation
There was a problem hiding this comment.
Left one comment where I wasn't sure about a line.
My only other question is this: Is there any way we can use an existing event emitter? I know QUnit doesn't like to have hard dependencies, but could Rollup be configured to bundle in certain dependencies and then we wouldn't have to roll our own EventEmitter?
| @@ -60,7 +60,7 @@ export default ( function() { | |||
| parse: function( obj, objType, stack ) { | |||
| stack = stack || []; | |||
| var res, parser, parserType, | |||
| inStack = inArray( obj, stack ); | |||
| inStack = stack.indexOf( obj ); | |||
There was a problem hiding this comment.
Should this say stack.indexOf( obj ) !== -1?
There was a problem hiding this comment.
Nope, we need the actual index value later. Renamed this variable to make it clearer as to what is going on.
I thought about this, but we don't need a super robust eventing system and we would still likely need to wrap the library in some of our own logic to verify usage according to what we expect. Therefore, since the actual code is only like 30 lines, I figured we should just maintain it ourselves. Seem good? |
| if ( inStack !== -1 ) { | ||
| return "recursion(" + ( inStack - stack.length ) + ")"; | ||
| if ( objIndex !== -1 ) { | ||
| return "recursion(" + ( objIndex - stack.length ) + ")"; |
There was a problem hiding this comment.
This would probably work better as a template literal, since you're using them elsewhere in this PR.
There was a problem hiding this comment.
Good point! Will update.
|
Updated. Thanks for the review. |
|
@platinumazure does this look good to you now? @leobalter any update about releasing 2.1.1? |
| } | ||
|
|
||
| return -1; | ||
| return array.indexOf( elem ) !== -1; |
There was a problem hiding this comment.
in a close future, hopefully, we'll be able to use [].includes.
|
|
||
| // Clone the callbacks in case one of them registers a new callback | ||
| let originalCallbacks = LISTENERS[ eventName ]; | ||
| let callbacks = originalCallbacks ? [ ...originalCallbacks ] : []; |
There was a problem hiding this comment.
It doesn't improve readability, but I was playing with the syntax: [ ...(originalCallbacks || []) ].
I still prefer your code. 👍
|
LGTM |
Do NOT merge until after 2.1.1 is released.
This implements a simple event emitter interface that will eventually supersede the current logging callbacks. No tests are added here since the
emitfunction is private in scope and not currently wired up internally. One goal here is to avoid a giant PR that both implement the event emitter and wires it up to all possible events.A follow-up commit to this will use the API implemented here to emit data according to the js-reporters standard.