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: implement Internet Identity example recommendations #1033

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

krpeacock
Copy link
Contributor

Here are some recommendations I have on best practices in the II example project.

Important and functional changes

  • AuthClient create should not be invoked during the login flow
    • Awaiting async behavior during the click event leads browsers to treat the interaction as a popup and reject it. This is a particular footgun for Safari mobile
    • fix: move AuthClient.create() into the global scope, and then update the Actor globally
  • Disable login button and greet button until the authClient is ready
  • Remove Promise wrapping of login
    • I get that await is fancy, but the actual browser API is based around callbacks and events, and onSuccess and onError are adequate.
    • Note: I would also accept the wrapping if it at least rejected in the onError callback

Less important (semantic HTML, a11y, and style)

  • Using window.greetActor and window.authClient instead of local variables so new devs can play with the objects in the console. We DO NOT have to accept this change - it's just an idea I had
  • The form elements are unnecessary without input elements, and don't add any semantic value. I prefer <button type="button"> over an empty form, and then we don't need to handle submit events

@krpeacock krpeacock requested a review from a team as a code owner October 24, 2024 19:28
window.authClient.login({
identityProvider: process.env.II_URL,
onSuccess: () => {
setActor(window.authClient);

Choose a reason for hiding this comment

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

Do you remove the login button onSuccess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I didn't notice that logic in the original version


let actor = greet_backend;
// Global variables that we will set up
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
// Global variables that we will set up
// Global variables for convenience while you're becoming familiar with the interface. You should use local variables or closures in production

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.

2 participants