-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
e135cdb
to
eda4b07
Compare
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.
Left some comments. Tests look good.
@@ -92,7 +92,7 @@ export class ConsoleStatsExporter implements types.StatsEventListener { | |||
* @param view recorded view from measurement | |||
* @param measurement recorded measurement | |||
*/ | |||
onRecord(view: View, measurement: Measurement) { | |||
console.log(`Measurement recorded: ${view.measure.name}`); | |||
onRecord(views: View[], measurement: Measurement) { |
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.
Update JSDocs here and elsewhere.
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.
Updated, thanks for noticing.
this.registerView(view); | ||
return view; | ||
} catch (err) { | ||
throw (err); |
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.
Do we need this try
-catch
block?
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.
Hmm not really. This was used in another approach I was trying and it ended up staying. Thanks for noticing, removed.
} | ||
|
||
/** | ||
* Registers an exporter to send stats data to a service. | ||
* @param exporter An stats exporter | ||
*/ | ||
registerExporter(exporter: StatsEventListener) { | ||
throw new Error('Not Implemented'); | ||
this.statsEventListeners.push(exporter); |
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.
Should we immediately notify it of all currently registered views?
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.
Definitely! I hadn't considered this case but it is really important! Thanks for the suggestion.
* feat: add stats implementation * refactor(fix): changes to address review comments
* feat: add stats implementation * refactor(fix): changes to address review comments
* feat: add stats implementation * refactor(fix): changes to address review comments
No description provided.