-
Notifications
You must be signed in to change notification settings - Fork 57
refactor: Migrate Identity API Client to TypeScript #948
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
refactor: Migrate Identity API Client to TypeScript #948
Conversation
7a613e8 to
8a2e225
Compare
rmi22186
left a comment
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.
looks great overall. the number of comments is high, but it's mostly the same thing over and over...if we are migrating to ts...let's keep everything typed!
rmi22186
left a comment
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.
minor nits
5864e6f to
cc84d6f
Compare
src/identityApiClient.ts
Outdated
| false | ||
| ); | ||
| } catch (err) { | ||
| const errorMessage = (err as Error).message || 'Unknown Error'; |
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 think Unknown error is too generic. While we will know it is only coming from here if we do a global search for it, it might be more helpful to provide more context.
I actually think here is a good instance to do something like a toString which you had before if there is no message. That will allow us to debug further. What do you think?
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 think I would prefer err.toString() if error.message is not available.
rmi22186
left a comment
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.
looks great!
b6883b6 to
b8a1a8f
Compare
fcb96db to
3d03859
Compare
|
68a0fee
into
refactor/ts-migration-blackout-2024


Instructions
developmentSummary
Testing Plan
Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)