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

How do the Response overloads actually work? #1040

Closed
domenic opened this issue Apr 13, 2017 · 16 comments
Closed

How do the Response overloads actually work? #1040

domenic opened this issue Apr 13, 2017 · 16 comments

Comments

@domenic
Copy link
Member

domenic commented Apr 13, 2017

Putting together JS.md + Web.md, we have the overloads:

Promise<WebAssembly.Module> compile(BufferSource bytes)
Promise<WebAssembly.Module> compile(Response source)
Promise<WebAssembly.Module> compile(Promise<Response> source)

How does the overload logic actually work, given an arbitrary object X?

In the Web IDL sense of overloads, these overloads are impossible impossible, because nothing is distinguishable with promises. That is, in most web APIs, any promise-based API causes the processing model to immediately convert the argument to a promise with Promise.resolve(arg), including BufferSources or Responses, so the Promise<Response> overload would always win.

My guess is that you are not using Web IDL's processing model for overloads, but it's not clear what processing model you are using.

I think the best processing model here that would fit with other web APIs would be the following:

function compile(arg) {
  return Promise.resolve(arg).then(arg => {
    if (isResponse(arg)) {
      ...
    } else if (isBufferSource(arg)) {
      ...
    } else {
      throw new TypeError(...);
    }
  });
}

However, if this is the intent, it's definitely not clear from the overload definitions. I would probably spec it as saying something like

On the web, the compile method is replaced with Promise<WebAssembly.Module> compile(Promise<Response or BufferSource> source), with the semantics that any incoming value is cast to a promise immediately. (This allows passing in Responses or BufferSources directly, as well as promises for them.)

Some notable test cases for this:

  • { then(f) { f(aResponseOrBufferSource); } should work
  • No sync errors are thrown ever; only rejected promises, including for invalid args like 5 or Promise.resolve(5).
  • If you have any way to observe whether compilation is sync or async, it'd be good to test that it's always async.
@rossberg
Copy link
Member

rossberg commented Apr 13, 2017 via email

@lukewagner
Copy link
Member

lukewagner commented Apr 13, 2017

Would changing the current signature to Promise<WebAssembly.Module> compile(Promise<Response or BufferSource> source) match any existing Promise-taking APIs and is this signature expressible in WebIDL? If not, or maybe regardless, since noone has shipped this particular overload, I think we could change it without breaking anything.

@domenic
Copy link
Member Author

domenic commented Apr 13, 2017

Would changing the current signature to Promise<WebAssembly.Module> compile(Promise<Response or BufferSource> source) match any existing Promise-taking APIs and is this signature expressible in WebIDL?

Yes and yes, assuming you include my

with the semantics that any incoming value is cast to a promise immediately. (This allows passing in Responses or BufferSources directly, as well as promises for them.)

The signature in Web IDL would be exactly that, in fact (including the automatic casting semantics).

@lukewagner
Copy link
Member

lukewagner commented Apr 13, 2017

Ah, ok, so at least we wouldn't be the odd one out. Would it be a valid "optimization" (not semantically observable) for an engine to observe it had been given a BufferSource and do no actual Promise.resolve(bufferSource)?

@domenic
Copy link
Member Author

domenic commented Apr 13, 2017

Not without great care. For example,

const u = new Uint8Array([0]);
u.then = f => f(new Uint8Array[1]);

should attempt to compile the bytes 0x01, not 0x00, since Promise.resolve(u) will result in a promise that fulfills with the 0x01 Uint8Array.

(Substitute appropriate valid wasm programs for my byte sequences as necessary.)

@lukewagner
Copy link
Member

Hah, good one. So then this would technically be a breaking change from what's currently shipping ;-) Could you perhaps point to any of the current uses of overloads based on the resolved value of a promise?

@rossberg
Copy link
Member

rossberg commented Apr 14, 2017

@lukewagner, I think it's worse than that: it would be a permanent incompatibility between the core JS API and its Web-enabled version. Unless we change the core version to take a promise as well, that is, but maybe you were implying that?

@domenic
Copy link
Member Author

domenic commented Apr 14, 2017

Could you perhaps point to any of the current uses of overloads based on the resolved value of a promise?

I cannot. Overloading in web APIs is rare, and web APIs that accept a promise (as opposed to returning it) are rare. Combined I do not believe there are any examples.

However, if you do intend to combine overloading + accepting a promise, the pattern I've given is the only way to do so in Web IDL-based APIs.

I think it's worse than that: it would be a permanent incompatibility between the core JS API and its Web-enabled version

  1. How would this be observable? Er, obviously it's observable per my above example
  2. If it is observable, what would be bad about such a difference in API?

@lukewagner
Copy link
Member

but maybe you were implying that?

Yes, that's what I was implying.

I think it's definitely important to be WebIDL-compatible. And not just on philosophical grounds: once we have the ability to import WebIDL types and functions using those types, this would be how wasm on the web could fetch-and-compile-and-instantiate from wasm itself w/o JS glue. And for that reason, I think using a more plain-vanilla WebIDL signature would be better.

So I think we should really consider making the response overloads separate methods. We could even use this as an opportunity to "fix" the #999 wart in the new response functions by having two.

@lukewagner
Copy link
Member

So to be concrete, the option I'm mildly in favor of (which splits out functions and fixes #999) is:

  • Promise<Module> compileResponse(r)
  • Promise<Instance> instantiateResponse(r, imports)
  • Promise<{module:Module,instance:Instance}> compileAndInstantiateResponse(r, imports)

where all three methods unconditionally Promise.resolve(r) their argument and reject if that doesn't resolve to a Response. I also realized that splitting out the overload allows much easier feature-testing during the interim when this feature isn't available everywhere.
(cc @flagxor @pizlonator for visibility)

@domenic
Copy link
Member Author

domenic commented May 1, 2017

Although I am hugely in favor the compileAndInstantiate split, I think it'd be confusing to fix that only for the Response variants. Ideally it could be fixed for both.

I still think just having the web specification override the definition of compile/instantiate is pretty reasonable.

I think there's a larger question about whether this API should be Web IDL-compatible, and if so, why it isn't just written in Web IDL. See also whatwg/webidl#226. But probably that can be separated.

@cynthia
Copy link

cynthia commented May 9, 2017

We at TAG have been looking into the document referenced in w3ctag/design-reviews#167 - if this change affects the proposal we have been looking into, I think we should be reviewing whatever is most likely to ship rather than a outdated document. We (including @slightlyoff and @travisleithead) would be more than happy to sit down and actively work with you on this.

If there are any updated points we should be looking into, please let us know!

@lukewagner
Copy link
Member

(Sorry, was offline for a bit there.) After some more discussion, agreed with @domenic that it'd be more irregular to have compileAndInstantiate only for the Response methods. Someone suggested using "streaming" instead of "response" to name the new method, so then the overloads would turn into two new functions:

  • Promise<Module> WebAssembly.compileStreaming(r)
  • Promise<{module:Module,instance:Instance}> WebAssembly.instantiateStreaming(r)

@cynthia Happy to work more with you all on this. The Web.md linked to in that review is still current, so if everyone agrees on this, I can write a PR to update Web.md.

@flagxor
Copy link
Member

flagxor commented May 18, 2017

Let's do this.
Sent out a change:
#1068

@flagxor
Copy link
Member

flagxor commented May 18, 2017

PTAL

Revised to address @domenic 's concern + using the syntax suggested by TAG.

@flagxor
Copy link
Member

flagxor commented May 23, 2017

Seems like this has been addressed by getting rid of the overload + explaining the promise handling imperatively in:
#1068

Closing.

@flagxor flagxor closed this as completed May 23, 2017
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

No branches or pull requests

5 participants