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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
May 9, 2024
Merged

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

merged 7 commits into from
May 9, 2024

Conversation

oscard0m
Copy link
Member

@oscard0m oscard0m commented May 8, 2024 •
edited
Loading

  • 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

@github-actions GitHub Actions
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.

Sign in to view
@gr2m

This comment was marked as resolved.

Sign in to view
gr2m
gr2m requested changes May 9, 2024
View reviewed changes
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.

Sign in to view
gr2m
gr2m reviewed May 9, 2024
View reviewed changes
test/typescript-validate.ts Outdated Show resolved Hide resolved
@wolfy1339

This comment was marked as resolved.

Sign in to view
@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
wolfy1339
wolfy1339 reviewed May 9, 2024
View reviewed changes
src/types.ts
@@ -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
@github-actions GitHub Actions
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 📦🚀

@gr2m gr2m mentioned this pull request May 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr2m gr2m gr2m approved these changes

@wolfy1339 wolfy1339 wolfy1339 approved these changes

Assignees
No one assigned
Labels
released Type: Feature New feature or request
Projects
🧰 Octokit Active
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

appId can now be set to the application's Client ID
3 participants
@oscard0m @wolfy1339 @gr2m