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
Convert OAuthClient to TypeScript and improve documentation #5328
Conversation
Convert oauth-client.js to TS and add additional documentation in the form of specification links and types for messages sent from the authorization popup window in h to the client.
Codecov Report
@@ Coverage Diff @@
## main #5328 +/- ##
==========================================
+ Coverage 99.17% 99.18% +0.01%
==========================================
Files 236 236
Lines 9375 9377 +2
Branches 2247 2247
==========================================
+ Hits 9298 9301 +3
+ Misses 77 76 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
clientId: string; | ||
tokenEndpoint: string; | ||
authorizationEndpoint: string; | ||
revokeEndpoint: string; |
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.
Not specific to this PR, but something that has popped my mind lately.
Would it make sense to set public properties in classes as readonly
? Assuming we don't want to be able to update them, of course, but when actually having a class, I would expect updates to happen via meaningful methods, if anything, and access to properties only with read purposes.
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.
Would it make sense to set public properties in classes as readonly?
Hmm... I suppose there is a tradeoff between the boilerplate of readonly
markers / getters for preventing re-assignment statically / dynamically vs the benefit of enforcing invariants. It will probably make sense to approach this differently depending on whether we are talking about a public API exposed to third parties vs. an internal API where we are only concerned about our own accidental misuse.
In this context the intent is that all the values are provided at construction time, although it should be safe to re-assign afterwards.
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 good.
Convert oauth-client.js to TS and add additional documentation in the form of specification links and types for messages sent from the authorization popup window in h to the client.