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

chore(providers): rewrite github provider in typescript #4908

Merged
merged 5 commits into from Jul 13, 2022

Conversation

mshd
Copy link
Contributor

@mshd mshd commented Jul 12, 2022

I rewrote the provider from js to ts and added the profile types.

I've tested it with two accounts, but please test it further as it's a critical provider.

☕️ Reasoning

Embrace typescript

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Please scout and link issues that might be solved by this PR.

Fixes: INSERT_ISSUE_LINK_HERE

📌 Resources

@vercel
Copy link

vercel bot commented Jul 12, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
next-auth ✅ Ready (Inspect) Visit Preview Jul 13, 2022 at 0:49AM (UTC)

@github-actions github-actions bot added core Refers to `@auth/core` providers labels Jul 12, 2022
Comment on lines +79 to +93
// If user has email hidden, get their primary email from the GitHub API
if (!profile.email) {
const emails: GithubEmail[] = await (
await fetch("https://api.github.com/user/emails", {
headers: { Authorization: `token ${tokens.access_token}` },
})
).json()

if (emails?.length > 0) {
// Get primary email
profile.email = emails.find((email) => email.primary)?.email
// And if for some reason it doesn't exist, just use the first
if (!profile.email) profile.email = emails[0].email
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is still needed. But probably somebody added it for a reason.

@vercel vercel bot temporarily deployed to Preview July 12, 2022 14:04 Inactive

// If user has email hidden, get their primary email from the GitHub API
if (!profile.email) {
const emails: GithubEmail[] = await (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added to the types to the email request too.

Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

@balazsorban44 balazsorban44 self-requested a review July 12, 2022 14:53
Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

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

Linting failed, other than that, looks good!

Co-authored-by: Balázs Orbán <info@balazsorban.com>
url: "https://api.github.com/user",
async request({ client, tokens }) {
// Get base profile
const profile = await client.userinfo(tokens.access_token!)
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/nextauthjs/next-auth/runs/7305230206?check_suite_focus=true#step:7:22

This throws as well. I believe it should always be string though, so maybe a // @ts-expect-error is forgivable here. If not, we can change it to throw an error in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vercel vercel bot temporarily deployed to Preview July 13, 2022 00:49 Inactive
Copy link
Member

@balazsorban44 balazsorban44 left a comment

Choose a reason for hiding this comment

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

Thanks!

@balazsorban44 balazsorban44 merged commit 3666e43 into nextauthjs:main Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Refers to `@auth/core` providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants