Replace incorrect wasm compilation fallback logic. Fixes #42055 partly #42918
Conversation
ghost
added
the
| if (!hasWasmContentType) { | ||
| // In most cases the developer should fix this. It's unusual enough that we don't mind logging a warning each time. | ||
| console.warn('WebAssembly resource does not have the expected content type "application/wasm", so falling back to slower ArrayBuffer instantiation.'); | ||
| } |
There was a problem hiding this comment.
we only attempt to use instantiateStreaming when the content-type application/wasm is found (as per spec), then it should work, and if not there's no reason to think that instantiate would be more successful.
In that case, should we just throw an exception given we don't expect the subsequent array buffer instantiation to work if !hasWasmContentType?
There was a problem hiding this comment.
Non-streaming compilation should work regardless of the content type. It’s only streaming compilation that has the content type prerequisite.
There was a problem hiding this comment.
I think the way I phrased the PR description was ambiguous and perhaps misleading. Hopefully the code itself does a better job of explaining!
TanayParikh
left a comment
TanayParikh
left a comment
There was a problem hiding this comment.
Thanks for the clarification! LGTM
Previously we:
instantiateStreaminginstantiateThe problem is that this kind of fallback has never worked, because the first call to
instantiateStreamingconsumes the response body, so the second step would be guaranteed to fail in all cases.We could solve this using
response.clone(), but there's no known use case for the fallback anyway. As long as we only attempt to useinstantiateStreamingwhen the content-typeapplication/wasmis found (as per spec), then it should work, and if not there's no reason to think thatinstantiatewould be more successful.So as of this PR, we don't do fallback - we just pick between the two compilation APIs based on the content-type. This should increase our compatibility versus what we did before.
Fixes one of the main concerns raised in #42055