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

More JSON algorithms #338

Merged
merged 2 commits into from
Oct 5, 2020
Merged

More JSON algorithms #338

merged 2 commits into from
Oct 5, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented Oct 1, 2020

The first commit here fixes #336. The second is a followup to help the section be more uniform and useful. They're likely best reviewed separately.


Preview | Diff

<a>converting an Infra value to a JSON-compatible JavaScript value</a>, given |value|.

<li>
<p>Return ! [$Call$](<a>%JSON.stringify%</a>, undefined, « |jsValue| »).
Copy link
Member

Choose a reason for hiding this comment

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

The undefined here is the this value, and I thought it looked unconventional to use undefined rather than null, but it actually seems idiomatic. https://tc39.es/ecma262/#sec-string.prototype.replace does it, for example.

<li><p>Let |i| be 0.

<li>
<p><a for=list>For each</a> |listItem| of |value|:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we don't have the equivalent of for (const [i, listItem] of value.entries()) for Infra lists. This works, though, I can't spot any problem at least.


<li><p>Return ? <a abstract-op>Call</a>(<a>%JSON.parse%</a>, undefined, « <var>jsonText</var> »).
<ol>
<li><p>Let |string| be the result of running <a>UTF-8 decode</a> on |bytes|. [[!ENCODING]]
Copy link
Member

Choose a reason for hiding this comment

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

Is the error handling of https://encoding.spec.whatwg.org/#utf-8-decode wired up correctly? It looks like decoders return errors, nothing is thrown, but "UTF-8 decode" doesn't handle the return value. Maybe that just means that replacing with U+FFFD is the only error handling mode one can use through "UTF-8 decode"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it looks like by default, the error mode is "replacement". You can use "UTF-8 decode without BOM or fail" to get actual errors.

Copy link
Member

@andreubotella andreubotella left a comment

Choose a reason for hiding this comment

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

I think it'd be better to have all Infra types be serializable, even if they don't round-trip.

@domenic
Copy link
Member Author

domenic commented Oct 2, 2020

I think it's better not to stretch things so that bytes/code points/byte sequences/structs/maps-with-non-string-keys become serializable somehow. The goal isn't to provide a mapping between Infra and JSON. It's to allow you to set up specially-crafted Infra values, and turn them into JSON.

I'd be happy to revisit that if we had concrete use cases, but I suspect that people who are creating Infra values with an eye toward JSON serialization will just not use the unserializable types.

@foolip
Copy link
Member

foolip commented Oct 3, 2020

For WebDriver BiDi we will indeed be crafting Infra values specifically to pass to this algorithm, we won't be passing in values that we also use for some other internal purpose.

@andreubotella
Copy link
Member

andreubotella commented Oct 3, 2020

Fair enough. Since stacks, queues and sets are subsets of list, they could still be serialized, but that's probably not worth disallowing.

I've deleted the comments relating to serialization of further types above, leaving the typo patch.

* Adds "Parse a JSON string into a JavaScript value" as a thin wrapper
  around %JSON.parse%.

* Adds "serialize a JavaScript value to a JSON string", allowing
  standards to avoid the unexpected behavior of %JSON.stringify% when
  used on non-JSON-serializable JavaScript objects.

* Renames "parse JSON from bytes" to "parse JSON bytes to a JavaScript
  value", and "serialize JSON to bytes" to "serialize a JavaScript value
  to JSON bytes". (lt="" values for the old names are kept, so as not
  to break links.)

* Renames "parse JSON into Infra values" to "parse a JSON string to an
  Infra value" (again keeping the old lt="" value).

* Adds "parse JSON bytes to an Infra value".

* Minor tweaks to source syntax to uniformize on [$$] and ||.

* Minor tweaks to variable names to uniformize on "string", "bytes",
  "jsValue", and "value".

* Ensures that all exported terms in this section also have their gerund
  forms exported.
@domenic domenic merged commit d91d797 into master Oct 5, 2020
@domenic domenic deleted the infra-to-json branch October 5, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Serialize Infra values to JSON bytes
4 participants