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

Stronger typing for screen_hint property #912

Merged
merged 5 commits into from
Jun 14, 2022

Conversation

iAmWillShepherd
Copy link
Contributor

@iAmWillShepherd iAmWillShepherd commented May 28, 2022

I noticed that this property has type string which doesn't help guide me or others to the pit of success. Given this property can only be one value, I've taken the liberty of updating your types so Typescripts users can continue to have a great developer experience using Auth0.

After merging this change, you should notice you get a helpful hint for what values are acceptable.

Screen Shot 2022-05-27 at 8 14 13 PM

@iAmWillShepherd iAmWillShepherd requested a review from a team as a code owner May 28, 2022 01:08
@frederikprijck
Copy link
Member

Thanks for the PR. I believe screen_hint can be more than just one value.

Looking at the source code of Auth0's server, I can see it for sure supports both signup and login. I understand login is the same as not sending any value at all. However, it's a valid option.

That said, I can see that the Auth0 server does not put any limitation on what you can send as a screen_hint, it basically allows to send any string, but fallback to login when you send anything that isn't login or signup.

Even tho I can see that might be weird, we might need to ensure we don't break people (previously, people sending screen_hint=foo should have been the same as screen_hint=login. Again, this is weird, but looking at the source code of Auth Server, this is expected.

@stevehobbsdev What do you think? I am always in favor of improving the experience for our users. But we need to ensure we don't break them.

@iAmWillShepherd
Copy link
Contributor Author

I can update the type to include loginas well.

@stevehobbsdev
Copy link
Contributor

Why don't we type it as 'signup' | 'login' | string so that we're able to get the suggested values, plus the ability to send any string if the developer needs to?

@iAmWillShepherd
Copy link
Contributor Author

Why don't we type it as 'signup' | 'login' | string so that we're able to get the suggested values, plus the ability to send any string if the developer needs to?

I went ahead and made the change, but I think that test should be updated to accept the values your docs mention. Typing to string as a fallback doesn't feel like a great solution since your system doesn't care about just any string.

@frederikprijck frederikprijck merged commit cf038ad into auth0:master Jun 14, 2022
@frederikprijck frederikprijck mentioned this pull request Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants