fix: align generateAccessToken types with implementation#15
fix: align generateAccessToken types with implementation#15obarcelonap wants to merge 6 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
TokenParams now accepts {} — it loses the guarantee this PR is trying to preserve
Making clientId/clientSecret/orgId all optional and adding [key: string]: unknown means an empty object now type-checks, even though the runtime throws MISSING_PARAMETERS unless one casing of each of the three is present (ims.js:63-74). The motivation section says consumers shouldn't "lose type safety," but this change removes the compile-time signal that credentials are required.
A union models the camelCase-or-snake_case requirement without dropping the guarantee:
type Credentials =
| { clientId: string; clientSecret: string; orgId: string }
| { client_id: string; client_secret: string; org_id: string }
export type TokenParams = Credentials & {
scopes?: string[]
// injected by the include-ims-credentials annotation
__ims_oauth_s2s?: Record<string, unknown>
__ims_env?: string
}If the index signature is really only there to admit the __ims_* annotation keys, declaring them explicitly (as above) is more honest than [key: string]: unknown, which also defeats excess-property checking on typos like clinetId.
@MichaelGoberling could use your input here too
|
Yes I think we should be explicit about whats supported, instead of using the unknown property Tbh this seems to have been an interface miss. I would have preferred to settle on only one of camel case or snake case, instead of both. But its out there now and probably not worth the breaking change |
|
@pru55e11 @MichaelGoberling thanks for the quick review :) The new proposal tries to be accurate to the actual implementation using a union of the valid credential forms (at the end the impl uses always |
Description
types.d.tswas missing the optionalimsEnvsecond parameter from thegenerateAccessTokensignature. While fixing that, a few other mismatches were also corrected:TokenParamsdeclaredenvironment?: 'prod' | 'stage'which the implementation never readsTokenParamsis now a union of the three valid credential forms — camelCase, snake_case, and__ims_oauth_s2s— faithful to howgetAndValidateCredentialsactually resolves credentialsRelated Issue
N/A
Motivation and Context
Consumers relying on the published types would get incorrect type errors for the missing second argument, and the credential types didn't reflect what the implementation actually accepts.
How Has This Been Tested?
Existing test suite — 67 tests, 100% coverage — passes unchanged.
Screenshots (if appropriate):
Types of changes
Checklist: