feat(js): add check to no_modules gen js for cloudflare workers

[ashleygwilliams]

No description provided.

[kentonv]

For the record, the reason for this strange behavior is that the module comes from a different JS context, and the primordials in each context are, unfortunately, considered to be different objects. So it is an instance of WebAssembly.Module, but not the calling context's WebAssembly.Module.

A similar issue would happen in browsers when passing modules between (same-origin) windows or frames.

[alexcrichton]

Thanks for the PR @ashleygwilliams and explanation @kentonv!

I wonder if we might be best getting by by inverting the conditional though? I think that WebAssembly.instantiate is actually pretty flexible with what it takes, but it is basically guaranteed to not be able to take a string at least. For example we could probably enable more use cases where if a Uint8Array were passed in as well it'd go straight to instantiation instead of trying a fetch.

I think that'd also have the nice bonus of solving the cloudflare issue b/c the object passed in definitely isn't a string, so it'd hit the final branch of WebAssembly.instantiate easily

[ashleygwilliams]

@alexcrichton yeah i definitely agree, good call. i'll update this PR.

[kentonv]

@alexcrichton What if someone is passing in an instance of URL? JavaScript's automatic coercion to strings is pretty unfortunate... :/

[alexcrichton]

Hm that's true yeah, and switching may end up breaking someone, but I kinda like the sound of "special case going to fetch otherwise default to WebAssembly.Module" in the sense that if necessary we could add cases for URL and other JS classes if necessary. Or in other words, the fetch case seems like it should be the exception, not the rule to instantiation (with WebAssembly.instantiate being the "rule"

[ashleygwilliams]

@alexcrichton yeah i think that is why i also felt it was a good idea. it's definitely the case that instantiate should be the "primary" and "fetch" is the conditional.

i'd also want to avoid a large set of conditionals. that being said, we might be moving into "different feature" territory. how do folks feel about me fixing this PR to pass and then filing and fixing this expressivity issue in a separate PR? (we can go through and think through all the cases there)

[alexcrichton]

I think I'd personally still prefer to switch the order of the clauses, executing fetch if the input argument is a string checking typeof, and then using WebAssembly.instantiate for everything else. If it causes too much breakage we can revert, otherwise I'm pretty sure we can handle other cases like URL as they arise

[kentonv]

Oh, I forgot about a more obvious one: Request is a type specifically intended to be passed to fetch().

Maybe just check for URL and Request in addition to string? Those three are the only types I commonly see being passed to fetch(). I agree fetch() shouldn't be the default.

(Obviously, I'm just a spectator here so also feel free to ignore me.)

[ashleygwilliams]

@kentonv one of us! one of us! spectators are extremely welcome. no need to apologize! i think that's a great plan and i'm working on it now.

@alexcrichton this test sitch.. looks like these errors are unrelated to the PR? i think you're working on this actively right now, but lemme know if i'm mistaken.

[alexcrichton]

👍

[alexcrichton]

Ah yeah the errors are unrelated to this, so no worries about those

[RReverser]

Just for the record, another option could've been to use a string tag-based check that is commonly used in JS for such cross-realm checks:

Object.prototype.toString.call(module_or_path) === '[object WebAssembly.Module]'

[ashleygwilliams]

@RReverser yeah that was the original plan, but in talking with @alexcrichton we realized that we much preferred making the conditional prioritize the more exceptional case (fetch), and letting instantiate be the catch all (as it's the more general case).