-
Notifications
You must be signed in to change notification settings - Fork 93
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
Add "parse JSON into maps and lists" #249
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.
This seems like a good first step. @marcoscaceres might also appreciate this although I think he mentioned wanting to define things in terms of JavaScript somewhere. Perhaps this can sway him.
I don't mind adopting more Bikeshed bits. I used to care a lot about the generated document as that's what folks view source on, but that became a lost cause quite a while ago. Perhaps we can still salvage that somehow though. As we adopt more Bikeshed bits, a risk is that we make contributing to HTML even harder, as it'll be so unfamiliar to folks, in particular long term.
infra.bs
Outdated
standards, it is often more convenient to parse JSON into realm-independent <a>maps</a>, | ||
<a>lists</a>, <a>strings</a>, <a>booleans</a>, numbers, and nulls. | ||
|
||
<p>To <dfn export>parse JSON into maps and lists</dfn>, given a <a>string</a> |
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.
It's weird that this also returns primitives. "parse JSON into Infra values" might be good enough?
Indeed... I quite like this!
With this PR, I don't think doing the JS conversion is necessary, anymore - because the infra types map quite ergonomically to JS types and to C++ types. Concretely, for the use case I have in mind (web manifest), the Gecko implementation is in JS, but Chrome's implementation is in C++. Thus, Infra will give us language/type neutrality, which is a huge plus. The things we then build onto of this can then operate on the infra types 👍🔥👍 |
<p>To <dfn export>parse JSON into Infra values</dfn>, given a <a>string</a> <var>jsonText</var>: | ||
|
||
<ol> | ||
<li><p>Let |jsValue| be ? [$Call$](<a>%JSONParse%</a>, undefined, « |jsonText| »). |
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.
I wonder if it would be more useful for callers if this returned failure upon throwing.
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.
I think it's easier if callers can just re-throw the specific JSON parsing error, instead of having to remember that it's a SyntaxError, and using prose to stitch the error messages together. At least, that is what would work well for me in import maps.
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.
Okay, I was wondering if maybe returning failure would be better so you don't have to catch, but if you want to re-throw that makes sense.
Part of #159. This provides the basic parsing framework, although no validation or type-checking.
Part of #159. This provides the basic parsing framework, although no validation or type-checking.
Questions:
Comment for @annevk: I used Bikeshed shorthands here because I find them way more pleasant than all the close and open tags for
<a>
and<var>
. I was tempted to use Bikeshed's built-in list and paragraph support as well, but I refrained. I realize this is inconsistent with the rest of the source text, and will change it if you so desire. I just urge you to look at the beauty that is the source of the import maps spec and consider what could be :).Preview | Diff