-
Notifications
You must be signed in to change notification settings - Fork 57
Conversation
(Accidentally posted before filling out the PR, somehow. Updated with title/description.) |
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.
Looks good to me. Wait until after the CG meeting to merge?
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 looks great! I'm a big fan of the design decisions to not include all of WebIDL and just start with some of the most important types (especially given @Ms2ger's ongoing work to remove unused legacy features from WebIDL) and to fall back to JS conversions when the WebIDL types don't match (given how, sometimes, specifications are refactored over time to have different IDL).
I'm looking forward to all of this proposal moving forward and becoming even more concrete. I can imagine many additional types of conversions making sense to add in conjunction with the GC proposal (e.g., passing strings in and out without copying).
Great to hear, thanks for the feedback. I'll hold off merging until a chance to discuss in the next CG meeting. |
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.
Agreed, this looks really good. It's much easier now to see how this all fits together, and where we can add more functionality to provide more expressive bindings. It'd be great to see more example bindings, including cases that aren't representable with the current binding expressions.
Very exciting!
This is probably worth calling out explicitly as this is indeed somewhat common, including changing long to double and such). I take it this works for both input and return values? Really cool tactic, though I guess that means there will be a little bit more overhead. Am I correct that the intention here is to always handle It might be worth considering to map |
@annevk Thanks for the comments; PTAL to see if they were addressed correctly.
I added a third bullet to "Question 2" in the "JS API Integration" section.
Yes, input and return values (both being part of the "Web IDL Signature" which can mismatch. This is an instantiation-time check that only takes the slower, roundabout path on mismatch, so hopefully no overhead in the common case.
Ah, good question; I glossed over this. So I think this only comes up for the
Good point! Thinking about it some more, |
Trapping is some kind of error? So if you store a lone surrogate in a Text node's data in JavaScript you cannot get it out through Wasm? Mapping the lone surrogates to U+FFFD seems preferable if so. cc @hsivonen |
We've been discussing the problem of lone surrogates recently with Our current thinking is that we will add a method to detect if a JS string is invalid utf-16 (aka has a lone surrogate), and then code that wants to be robust will do the moral equivalent of taking In that sense I think that I might agree with @annevk that using replacement characters on lone surrogates might make more sense since it matches |
To clarify a bit, when the user types a single character in an So when the first That's pretty terrible since now the string doesn't match what the user typed (and this then leads to other bugs). So personally I would prefer a hard error, but I agree with @alexcrichton that matching existing |
@Pauan is there an issue tracking that particular way of dealing with user input? That seems like something that ought to be fixed in implementations (and specification, if it's unclear). (And yeah, |
On our side we have rustwasm/wasm-bindgen#1348 (linked earlier by @alexcrichton). The solution we decided upon was to just completely ignore As for a tracking issue in browsers or the spec, not that I'm aware of. But then again, I haven't really looked.
If it could be fixed in browsers, then that would be fantastic! Right now we have to ignore
Yeah, that would be useful! Right now we have to use a function like this, but it can probably be implemented faster in browsers. |
Oops, right. Ultimately, I assumed the spec here would delegate to |
Thanks @chicoxyzzy! Co-Authored-By: lukewagner <[email protected]>
statement: | ||
|
||
```wasm | ||
(webidl-type $Contact (dict (field "name" DOMString) (field "age" long))) |
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.
IMO, the text format of this should be as close as possible to the 'official' syntax for webIDL itself.
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 consistency with the existing text format is also a useful property, but probably we can discuss this in a follow-up issue and iterate on the explainer.
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.
Can we try to uniformly render custom sections in the text format by means of the proposed custom annotation syntax? That would prevent all sorts of compatibility and tooling issues.
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.
Ah, I forgot all about that. So, iiuc, the request is to make sure that any new syntax is contained by some (@webidl ...)
, and then otherwise the ...
can be anything (as long as it follows the basic paren-matching rules)?
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.
Within limits. It has to be lexible as Wasm tokens. That covers a wide range of lexical syntax (because Wasm identifiers can be almost anything), but we'd need to extend the proposal slightly if we wanted to allow other kinds of brackets as well or "foreign" punctuation like commas and semicolons. Those wouldn't be a problem. (Though it would be a problem to allow completely different lexing rules, mainly because of parens in comments and strings.)
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.
(That said, I wouldn't recommend going wild with annotation syntax but rather keep the spirit of the surrounding Wasm syntax.)
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.
Heh, that makes sense, but for the two practical options we're considering here (wat-style or Web IDL), it sounds like we're fine, so the only real requirement is, whatever it is, it is seated in a (@webidl ...)
, not a (webidl...)
, right?
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.
Yep!
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.
Righto, will fix, thanks for pointing that out.
Following the unanimous poll at the last CG meeting and no objections since, I'll merge, rename the repo, and we can continue to iterate on the design going forward. |
In this case there is clear conflict: between the prior precedents of
webidl and that of WebAssembly.
IMO webidl should prevail because otherwise there will be a lot of
unnecessary reformatting needed. Furthermore we would end up having to
restate the webidl specification and also having to own that restatement.
On the other hand there is no binary representation of webidl and so we
have to invent it. In that case the binary format is consistent with
WebAssembly
On Thu, Apr 4, 2019 at 8:57 PM Andreas Rossberg ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In proposals/webidl-bindings/Explainer.md
<#21 (comment)>
:
> +used as Web IDL Types.
+
+As an example, the Web IDL Dictionary:
+
+```WebIDL
+dictionary Contact {
+ DOMString name;
+ long age;
+};
+```
+
+could be defined in a WebAssembly module with this (strawman text-format)
+statement:
+
+```wasm
+(webidl-type $Contact (dict (field "name" DOMString) (field "age" long)))
Can we try to uniformly render custom sections in the text format by means
of the proposed custom annotation syntax
<https://github.com/WebAssembly/annotations/blob/master/proposals/annotations/Overview.md>?
That would prevent all sorts of compatibility and tooling issues.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#21 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACAL0IrVlkpPwNDy21mZhDWLEhE3Bu83ks5vdsnAgaJpZM4cEhyN>
.
--
Francis McCabe
SWE
|
FWIW, the custom annotation syntax should probably be able to embed existing WebIDL syntax: (@webidl
dictionary Contact {
DOMString name;
long age;
};
) would be syntactically well-formed. Whether it's desirable stylistically I'll leave for others to decide. |
Good to know; probably best to open a new issue to discuss Web IDL vs s-expression syntax. |
I'd suggest not using the WebIDL surface syntax, as this has changed over time and is continuing to change. |
This PR updates the Host Bindings proposal as discussed at TPAC and after a bunch of follow-up discussions.
The PR has an overview that I won't restate but one additional proposed change that I'd like to make (if we accept this PR) is to rename this repo from "host-bindings" to "webidl-bindings", to reflect the new scope of the proposal (for reasons given in the FAQ of the PR).
To make sure we have proper visibility and discussion, I'll add an agenda item to the April 2 CG meeting for discussion.