-
Notifications
You must be signed in to change notification settings - Fork 84
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
Define API values in terms of WebIDL rather than custom JSON definitions #367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sending the PR! Looks great
@@ -1095,7 +1149,7 @@ requests. | |||
1. Let |provider| be |options|["{{CredentialRequestOptions/identity}}"]["{{IdentityCredentialRequestOptions/providers}}"][0]. | |||
1. Let |credential| be the result of running the [=potentially create IdentityCredential=] | |||
algorithm with |provider|. | |||
1. If |credential| is null, wait for the task that throws a {{NetworkError}}, otherwise return | |||
1. If |credential| is null or [=potentially create IdentityCredential=] threw a {{TypeError}}, wait for the task that throws a {{NetworkError}}, otherwise return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the throw semantics in spec, do we need to catch it here or is it ok for us to get rid of the comment here about error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also unsure of the semantics of throw in spec. We definitely need to have some consideration for it because the "convert" operation I use will throw a TypeError
if the JSON object is incompatible with the IDL.
Co-authored-by: npm1 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wonderful, will save so many lines not having to parse JSON manually!! Good stuff! Will let you resolve the remaining feedback and merge when you are through with them!
…rom w3c-fedid#362 Also, sneak in some comment -> Note changes
Thanks all! There is only one remaining discussion which I don't think we have an immediately obvious better answer for. I'm pleased if this lands with the changes I just pushed (f5179c3). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM++
I'll let @npm1 merge once you two wrap up the remaining comments.
Oh I was going to merge but this has a merge conflict (probably my bad) - can you resolve then we can merge? |
No problem- should be merge-able now. |
Resolves #347
Preview | Diff