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

Reduce user facing provider APIs #1022

Closed
balazsorban44 opened this issue Jan 1, 2021 · 4 comments · Fixed by #1023
Closed

Reduce user facing provider APIs #1022

balazsorban44 opened this issue Jan 1, 2021 · 4 comments · Fixed by #1023
Labels
enhancement New feature or request providers

Comments

@balazsorban44
Copy link
Member

balazsorban44 commented Jan 1, 2021

Currently, there are some "magical" 🧙 fields only utilized by specific providers. We could handle these edge cases internally instead. All providers have an id, which we can use to switch on different behaviors. It would also make it easier to keep a TypeScript provider interface simple, without the need for Discriminated Union or similar

Some of these fields and which provider(s) use them (introduced in which PR):

@balazsorban44 balazsorban44 added the enhancement New feature or request label Jan 1, 2021
@balazsorban44
Copy link
Member Author

balazsorban44 commented Jan 1, 2021

@cathykc As the original author of #895, I would like to ask you, how strongly do you feel about additionalAuthorizeParams being a provider property, instead of using the (new) third argument of the signIn client function?

const signIn = async (provider, args = {}, authParams = {}) => {
const baseUrl = _apiBaseUrl()
const callbackUrl = (args && args.callbackUrl) ? args.callbackUrl : window.location
const providers = await getProviders()
// Redirect to sign in page if no valid provider specified
if (!provider || !providers[provider]) {
// If Provider not recognized, redirect to sign in page
window.location = `${baseUrl}/signin?callbackUrl=${encodeURIComponent(callbackUrl)}`
} else {
let signInUrl = (providers[provider].type === 'credentials')
? `${baseUrl}/callback/${provider}`
: `${baseUrl}/signin/${provider}`
if (authParams) {
signInUrl += `?${new URLSearchParams(authParams).toString()}`
}

Example:

<button
  onClick={() => signIn("slack", {}, {user_scope: 'identity.basic,identity.email,identity.avatar'})}
>
  Login
</button>

Right now, there are 2 ways of doing it, and we could eventually keep both, I was just curious about your opinion.
Setting authorize params may actually be useful for several providers and can be a general thing in contrast to the other, above mentioned magical provider properties, so I may even be OK with removing the authParams from the client signIn function instead. I might want to rename it to just authParams or authorizeParams removing the additional prefix.

UPDATE:
After giving it some thought, I think it makes more sense to keep your change and remove it from the signIn client-side function.

@balazsorban44
Copy link
Member Author

@cathykc when #1023 will be merged, the name will change from

additionalAuthorizeParams to authorizationParams. This plays better with the already present authorizationUrl.

I changed the slack provider respectively, so no changes will be needed.

@github-actions
Copy link

github-actions bot commented Jan 5, 2021

🎉 This issue has been resolved in version 3.2.0-canary.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Feb 1, 2021

🎉 This issue has been resolved in version 3.3.0-canary.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request providers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant