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

Change return type of CredentialsContainer.create for WebAuthn #1742

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asmsuechan
Copy link

@asmsuechan asmsuechan commented Jun 19, 2024

Hi developers,

The return value for CredentialsContainer.create is one of FederatedCredential, PasswordCredential, and PublicKeyCredential
according to https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/create#return_value

The return types are subclass of Credential. So I changed using PublicKeyCredential from Credential. Note that FederatedCredential and PasswordCredential haven't been implemented yet, therefore, I didn't use them. I'll also try to create them after I learn the manners of this repo.

Thank you for reading my PR.

Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@asmsuechan
Copy link
Author

@microsoft-github-policy-service agree

@saschanaz
Copy link
Contributor

saschanaz commented Jun 19, 2024

This can be a problem when other types of credentials come, maybe there should be additional overload based on the value of credential?

@asmsuechan
Copy link
Author

@saschanaz Thank you for your comment! Yes, what you say is true. Promise<PublicKeyCredential | FederatedCredential | PasswordCredential | null> is the correct interface. However, FederatedCredential and PasswordCredential are not yet implemented. So I added only PublicKeyCredential because it's better than the current implementation Promise<Credential | null>.

If it's unacceptable, I'll add the other interfaces above in this PR. I would love to know the manners of this repo first.

@asmsuechan
Copy link
Author

asmsuechan commented Jun 20, 2024

Oops, FederatedCredential and PasswordCredential are supported only by Chrome.

check whether it's supported by two or more browser engines

According to the sentence, they don't meet the qualification.

So, Promise<Credential | PublicKeyCredential | null> is better for now?

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