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

fix(react): signIn provider type #2655

Merged
merged 6 commits into from Sep 7, 2021
Merged

Conversation

Patryks1
Copy link
Contributor

@Patryks1 Patryks1 commented Sep 3, 2021

Reasoning 💡

Currently the signIn provider type will only accept email and credentials. This pr will accept any string instead.

Checklist 🧢

  • Documentation
  • Tests
  • Ready to be merged

Affected issues 🎟

Fixes #2654

@vercel
Copy link

vercel bot commented Sep 3, 2021

@Patryks1 is attempting to deploy a commit to the NextAuth Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the core Refers to `@auth/core` label Sep 3, 2021
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.

Thank you for jumping on this!

Some thoughts.

Ideally I would like that whenever someone types signIn(" in their IDE, a list of suggestions come up to show all the providers we have built-in support for. This currently includes credentials, email and any of the OAuth providers' id.

Example:

Although we don't want to restrict the first param of signIn to these values in case someone creates a custom provider, with a unique id.

For this to work, I found the following workaround (taken from microsoft/TypeScript#29729 (comment)):

// https://github.com/microsoft/TypeScript/issues/29729#issuecomment-832522611
export type LiteralUnion<T extends U, U = string> =
  | T
  | (U & Record<never, never>)

export type RedirectableProviderType = "email"  |  "credentials"
export type BuiltInProviderType = LiteralUnion<RedirectableProviderType | OAuthProviderType>

OAuthProviderType should be a union of id strings from across our src/providers/* folder. We will have to generate this type, by iterating over that folder and use the file name, and put it in a file like src/providers/oauth-types.

Without LiteralUnion:

With LiteralUnion (while still getting the hints for the built-in providers.):

provider?: RedirectableProvider,
provider?: string,
Copy link
Member

Choose a reason for hiding this comment

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

This seems to destroy the purpose of RedirectableProvider.

What we would like here is to hint the user of any of the built-in provider types, but not restricting it to those.

@Patryks1
Copy link
Contributor Author

Patryks1 commented Sep 5, 2021

@balazsorban44 Any ideas on how you could possibly grab all the providers dynamically? I can only think of require.context/filesystem which might work in this scenario. however i dont think they will be done on compile which means they wont show up on intel sense. I could do it the dirty way and just add the providers by hand?

@balazsorban44
Copy link
Member

balazsorban44 commented Sep 5, 2021

I would use a node script to grab filenames.

For inspiration https://github.com/nextauthjs/docs/blob/main/scripts/generate-providers.js

Then call this script when the build script is run

@Patryks1
Copy link
Contributor Author

Patryks1 commented Sep 6, 2021

I would use a node script to grab filenames.

For inspiration https://github.com/nextauthjs/docs/blob/main/scripts/generate-providers.js

Then call this script when the build script is run

Nice one, Let me know if thats what you where looking for.

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.

So I had some suggestions, and since I wanted to get this PR as quickly as possible, I thought I was committing to your branch, but instead, it ended up on a totally different branch...

Could you incorporate this commit with the suggested changes to your branch?: 2d8270c

It is mostly moving things around a bit, you have done nice work!

package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@balazsorban44 balazsorban44 changed the title fix(react): singIn provider type fix(react): signIn provider type Sep 7, 2021
@balazsorban44 balazsorban44 changed the base branch from next to beta September 7, 2021 08:50
@codecov-commenter
Copy link

Codecov Report

Merging #2655 (1344e21) into beta (2cb763c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             beta    #2655   +/-   ##
=======================================
  Coverage   14.26%   14.26%           
=======================================
  Files          85       85           
  Lines        1339     1339           
  Branches      378      378           
=======================================
  Hits          191      191           
  Misses       1078     1078           
  Partials       70       70           
Impacted Files Coverage Δ
src/react/index.tsx 98.47% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cb763c...1344e21. Read the comment docs.

@balazsorban44 balazsorban44 merged commit 17bea4a into nextauthjs:beta Sep 7, 2021
@Patryks1 Patryks1 deleted the patch-2 branch September 7, 2021 09:39
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

3 participants