Skip to content

fix: align generateAccessToken types with implementation#15

Open
obarcelonap wants to merge 6 commits into
adobe:mainfrom
obarcelonap:fix/types-generate-access-token
Open

fix: align generateAccessToken types with implementation#15
obarcelonap wants to merge 6 commits into
adobe:mainfrom
obarcelonap:fix/types-generate-access-token

Conversation

@obarcelonap

@obarcelonap obarcelonap commented Jun 16, 2026

Copy link
Copy Markdown
Member

Description

types.d.ts was missing the optional imsEnv second parameter from the generateAccessToken signature. While fixing that, a few other mismatches were also corrected:

  • TokenParams declared environment?: 'prod' | 'stage' which the implementation never reads
  • TokenParams is now a union of the three valid credential forms — camelCase, snake_case, and __ims_oauth_s2s — faithful to how getAndValidateCredentials actually resolves credentials

Related 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

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pru55e11 pru55e11 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MichaelGoberling

Copy link
Copy Markdown
Contributor

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

@obarcelonap

Copy link
Copy Markdown
Member Author

@pru55e11 @MichaelGoberling thanks for the quick review :)
My initial approach was to be minimal, actually the PR started from a missing second arg imsEnv in the function signature. The [key: string]: unknown was just a quick way to accommodate the other cases without exposing the internals of the implementation.

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 getAndValidateCredentials). Worth to consider that the impl resolves each credential field independently so mixed casing technically works at runtime, but modelling that would be cumbersome, I believe proposed approach covers all realistic usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants