Rename Response based compile + instantiate *Streaming. #1068
Conversation
flagxor
requested review from
jfbastien,
domenic,
mtrofin,
rossberg,
MikeHolman and
lukewagner
|
Oh right, we also need to (in this PR or another) address @domenic's root issue in #1040 which is that ( |
flagxor
force-pushed
the
streaming
branch
2 times, most recently
from
597f1f2 to
c1d3317
Compare
|
PTAL Revised to address @domenic 's concern + using the syntax suggested by TAG. |
| @@ -113,7 +113,12 @@ If neither of the following overloads match, then the returned `Promise` is | |||
| with a [`TypeError`](https://tc39.github.io/ecma262/#sec-native-error-types-used-in-this-standard-typeerror). | |||
|
|
|||
| ``` | |||
| Promise<{module:WebAssembly.Module, instance:WebAssembly.Instance}> | |||
There was a problem hiding this comment.
Hmm, I guess the TAG suggested this change? I'm not sure it makes things clearer, given that you haven't switched wholesale to Web IDL (yet?), so it's just halfway between.
Probably WebAssembly.Module/Instance below, at the very least?
Web.md
Outdated
| ``` | ||
|
|
||
| Developers can set the argument `source` with either a promise that resolves | ||
| with a | ||
| `source` is unconditionally passed through `Promise.resolve(source)`. |
There was a problem hiding this comment.
You'll want to add "using the built-in value of Promise.resolve" just so that nobody thinks that monkeypatching Promise.resolve = function () { } should affect this implementation.
|
How about "{compile|instantiate}Stream" rather than "..Streaming"? "Streaming" suggests implementation detail, which may not be what necessarily happens. For example, if the engine doesn't AOT but only lazily JIT, "compileStreaming" would be incorrect. "Stream", on the other hand, refers to the parameter, and is factually true regardless of the implementation. |
|
Can folks please chime in on if we like compileStream + instantiateStream better. |
Unfortunately it is not true. A Response is not a Stream. It contains a ReadableStream as its But even if it were a Stream, we don't generally add the type name of arguments to the name of methods. E.g. we don't have |
|
Dominic makes a good point on the name. Leaving that as is. |
Clarify how the source parameter is resolved. Clarify how the result of instantiate* is described.
As per spec, (WebAssembly/design#1068), we don't have compile/instantiate overloads anymore, instead, we have explicitly named members. This change introduces the new APIs, implements instantiateStreaming based on compileStreaming, and uses the existing embedder mechanism. It does not yet remove the functionality from compile/instantiate - we do that after we adopt the new APIs on the blink side. Also, it temporarily handles exceptions on the v8 side, which is also something we'll move to the blink side. Bug: Change-Id: I77673b1c0d395dfcf13b2f25464fd5dfd99c8d82 Reviewed-on: https://chromium-review.googlesource.com/508852 Commit-Queue: Brad Nelson <[email protected]> Reviewed-by: Brad Nelson <[email protected]> Cr-Commit-Position: refs/heads/master@{#45411}
No description provided.