Skip to content

Fix CredentialsContainer.store() return type #1525

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

Closed

Conversation

lukewarlow
Copy link

The existing return type is wrong. This function returns an empty promise.

As can be seen in the specification and the MDN documentation.

https://w3c.github.io/webappsec-credential-management/#abstract-opdef-store-a-credential

https://developer.mozilla.org/en-US/docs/Web/API/CredentialsContainer/store

@github-actions
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.

@lukewarlow lukewarlow marked this pull request as draft March 18, 2023 13:22
@lukewarlow
Copy link
Author

lukewarlow commented Mar 18, 2023

Having ran it in the browser (tried Chrome with a password credentials) it also seems to return undefined?

const x = await navigator.credentials.store(new PasswordCredential({id: 'foo', password: 'foo', name: 'Foo'}));

@lukewarlow
Copy link
Author

lukewarlow commented Mar 18, 2023

I will close this for now and file an issue on the spec itself because I don't think it's types are correct. Chrome is the only browser I know of that actually implements this API in a meaningful way so the browser type declarations are probably just matching the spec IDL which is wrong.

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/credentialmanagement/credentials_container.cc#:~:text=ScriptPromise%20CredentialsContainer%3A%3Astore,ScriptPromiseResolver%3E(script_state)%3B Chrome's actual implementation doesn't match their IDL

@saschanaz
Copy link
Contributor

Sounds like that's a bug in Blink, maybe file a bug in https://bugs.chromium.org/p/chromium/ ?

@lukewarlow lukewarlow closed this Mar 18, 2023
@lukewarlow
Copy link
Author

@saschanaz idk if it requires manual work or if it will update automatically at some point but I've fixed this in the spec (not sure if it's published yet). It now correctly shows it returns an empty promise.

@saschanaz
Copy link
Contributor

It's not being recognized because of w3c/webappsec-credential-management#213.

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