-
Notifications
You must be signed in to change notification settings - Fork 9
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
Require JSON serialization checks "in and out" #179
base: main
Are you sure you want to change the base?
Conversation
<p> | ||
The object MUST only contain [=JSON type=] values. | ||
</p> |
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.
We probably don't need this. Serialization will throw automatically if it's not the case.
<p> | |
The object MUST only contain [=JSON type=] values. | |
</p> |
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.
Isn't this important for a reader of the spec to know? For example, someone working on OID4VP or a new protocol?
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.
Yes, but it's in the algorithm. It's redundant to state it twice.
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.
Where it should be defined is part of the registry rules. That is, for your protocol to be registered, it must be JSON stringifiable. That would let new protocols know that it's a requirement.
<li>If |requests| does not contain [=JSON type=] values, | ||
[=exception/throw=] a {{TypeError}}. | ||
</li> |
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 not right here... you need to:
- [=List/for each=] |request| of |requests|:
1.1. [=serialize a JavaScript value to a JSON string|Serialize=] |request| a to a JSON string. Rethrow any exceptions.
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.
@timcappalli, we also would need this on the way out, when readonly attribute object data;
is set... though there might not be enough spec text in the current spec to add this right now. However, it would be good to add something.
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.
Couple of small things...
Closes #125
The following tasks have been completed:
Implementation commitment:
Documentation and checks
Preview | Diff