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

Fix broken PublicKeyCredentialJSON WebIDL definition #1969

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

MasterKale
Copy link
Contributor

@MasterKale MasterKale commented Sep 20, 2023

Update the WebIDL definition of the shape of PublicKeyCredentialJSON based on feedback in #1958.

NOTE: I have no idea how to validate that this is actually going to fix the issue.

Fixes #1958.


Preview | Diff

@MasterKale
Copy link
Contributor Author

MasterKale commented Sep 27, 2023

I've added as reviewers individuals representing browser maintainers. I'd love to hear whether your IDL parsers can understand the WebIDL as-is, or not.

At the WG meeting today (9/27) I've heard the following about the ability to parse the current definition of PublicKeyCredentialJSON:

  • @agl said Chrome's IDL parser could handle it
  • @jschanck said Mozilla's IDL parser could not handle it

This PR is probably still relevant given we already have one project that can't parse it, which probably means the current definition is indeed invalid.

In addition, one that's unclear to me is if a comment is sufficient to communicate and constrain the desired shape of PublicKeyCredentialJSON that browsers take in and return from these methods.

Copy link

@jschanck jschanck left a comment

Choose a reason for hiding this comment

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

This is equivalent to what we implemented in Firefox. I don't have a strong feeling about the comment---IMO the text in Section 5.1 is already sufficient.

@pascoej
Copy link
Contributor

pascoej commented Oct 3, 2023

WebKit's IDL parser is able to handle the current definition of PublicKeyCredentialJSON.

@MasterKale MasterKale merged commit a824429 into main Oct 4, 2023
1 check passed
@MasterKale MasterKale deleted the mm-1958-fix-publickeycredentialjson-webidl branch October 4, 2023 19:43
Copy link
Contributor

@agl agl left a comment

Choose a reason for hiding this comment

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

Not thrilled, but it seems that the original was technically incorrect so we'll try this. Hopefully Typescript etc don't have too much of a problem.

github-actions bot added a commit that referenced this pull request Oct 4, 2023
…-webidl

SHA: a824429
Reason: push, by MasterKale

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

PublicKeyCredentialJSON is an invalid WebIDL construct
4 participants