Skip to content
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

feat: appId argument can be set to Client ID string #606

Merged
merged 7 commits into from
May 9, 2024

Conversation

oscard0m
Copy link
Member

@oscard0m oscard0m commented May 8, 2024

  • feat(types): appId allows string or number as type
  • test(types): add test coverage for appId new type

Resolves #603


Before the change?

appId only allowed number as type.

After the change?

appId allows number or string as type.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

Copy link
Contributor

github-actions bot commented May 8, 2024

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@oscard0m oscard0m requested a review from gr2m May 8, 2024 22:27
@oscard0m oscard0m added the Type: Feature New feature or request label May 8, 2024
@oscard0m oscard0m changed the title appid allows string or number feat(types): appId allows string or number as type May 8, 2024
@oscard0m oscard0m marked this pull request as ready for review May 8, 2024 22:28
@wolfy1339

This comment was marked as resolved.

@gr2m

This comment was marked as resolved.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

can you also remove

auth-app.js/src/index.ts

Lines 34 to 38 in 81ac6ec

if (!Number.isFinite(+options.appId)) {
throw new Error(
"[@octokit/auth-app] appId option must be a number or numeric string",
);
}
and cover the change of behavior? It is technically a breaking change in behavior, but in this particular case I'd suggest we do a fix release, as it reflects a change in GitHub's API.

@oscard0m

This comment was marked as outdated.

test/typescript-validate.ts Outdated Show resolved Hide resolved
@wolfy1339

This comment was marked as resolved.

@oscard0m oscard0m requested review from gr2m and wolfy1339 May 9, 2024 17:31
@oscard0m oscard0m force-pushed the appid-allows-string-or-number branch from 5a92718 to c6189b7 Compare May 9, 2024 17:34
@@ -152,7 +152,7 @@ export type UTC_TIMESTAMP = string;
export type AppAuthentication = {
type: APP_TYPE;
token: JWT;
appId: number;
appId: number | string;
Copy link
Member

Choose a reason for hiding this comment

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

For a later date, maybe we could use template literal strings to better define the format

Suggested change
appId: number | string;
appId: number | `Iv1.${string}`;

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this would imply a change in universal-github-app-jwt first. Do you want me to open an issue there first?

Copy link
Contributor

Choose a reason for hiding this comment

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

appId: number | Iv1.${string};

I wouldn't do that, because there is a chance that future Client IDs will start with lv2.... and I also think there are old apps that still have a client ID without the lv1 prefix

@gr2m gr2m changed the title feat(types): appId allows string or number as type feat: appId argument can be set to Client ID string May 9, 2024
@gr2m
Copy link
Contributor

gr2m commented May 9, 2024

@oscard0m if it's done from your side, can you let me merge it please? I want to test out the latest semantic-release beta version in a real-life environment

@oscard0m
Copy link
Member Author

oscard0m commented May 9, 2024

@oscard0m if it's done from your side, can you let me merge it please? I want to test out the latest semantic-release beta version in a real-life environment

go for it @gr2m

@gr2m gr2m merged commit 7dc08e5 into main May 9, 2024
6 checks passed
@gr2m gr2m deleted the appid-allows-string-or-number branch May 9, 2024 19:58
Copy link
Contributor

github-actions bot commented May 9, 2024

🎉 This PR is included in version 7.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@joebowbeer
Copy link

@oscard0m should I create an issue to update the README?

By my reading, the appId is required and clientId is optional, which confused me.

I didn't see any mention that clientId could be assigned to appId.

@gr2m
Copy link
Contributor

gr2m commented Aug 8, 2024

@oscard0m should I create an issue to update the README?

that would be great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

appId can now be set to the application's Client ID
4 participants