microsoft / TypeScript-DOM-lib-generator

Tool for generating dom related TypeScript and JavaScript library files
Apache License 2.0
600 stars 417 forks source link

Change return type of CredentialsContainer.create for WebAuthn #1742

Open asmsuechan opened 2 weeks ago

asmsuechan commented 2 weeks ago

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.

github-actions[bot] commented 2 weeks ago

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 commented 2 weeks ago

@microsoft-github-policy-service agree

saschanaz commented 2 weeks ago

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

asmsuechan commented 2 weeks ago

@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 commented 2 weeks ago

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?