Support for push report metrics endpoint. #98
Conversation
There was a problem hiding this comment.
Thanks @lisaah for this PR! Just a few notes for types and backwards compatibility
lib/regions.ts
Outdated
|
|
||
| constructor(trackUrl: string, apiUrl: string) { | ||
| constructor(trackUrl: string, apiUrl: string, trackPushUrl: string) { |
There was a problem hiding this comment.
To maintain backwards compatibility, this should be optional. I doubt anyone is creating their own Regions, but just in case!
lib/track.ts
Outdated
| @@ -20,6 +20,7 @@ export class TrackClient { | |||
| request: Request; | |||
| trackRoot: string; | |||
| apiRoot: string; | |||
| trackPushRoot: string; | |||
There was a problem hiding this comment.
Similarly, this should be optional
lib/track.ts
Outdated
| @@ -98,6 +100,10 @@ export class TrackClient { | |||
| }); | |||
| } | |||
|
|
|||
| trackPush(data: RequestData = {}) { | |||
There was a problem hiding this comment.
A few things here:
datashould be more strongly typed. According to the api docs, there are only four valid parameters. Atypeorinterfaceshould be created fortrackPushto only allow those four (requried) parameters- There should be special handling if
this.trackPushRootisundefined. Probably aMissingParamErroror a newErrorsubclass
There was a problem hiding this comment.
👍 Added the typing, can make it required but the docs don't seem to annotate them as required like it does for name in regular events.
|
@lisaah thanks for the updates! I'm sorry about the back and forth, but after you opened this PR I had a chat with the team about why the push metrics endpoint was different, and we all agreed it was a bit weird. We've made a change in production that adds |
Sure, think it makes the code cleaner that way anyways. |
|
Thanks again @lisaah, this has been published with |
The base url for this endpoint doesn't seem to obey the
api/v1standard of the rest.