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

gen-host-js: instance ESM output by default #406

Merged
merged 11 commits into from
Oct 28, 2022

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 28, 2022

Outputs a direct ES module from the JS host generator by default that can be directly imported in JS environments.

For the custom instantiation API that was the previous output, a new --instantiation flag is provided.

Instance-level customization flags are also added including:

  • --compat: Enables all compatibility mode flags (right now just --nodejs-compat, but based on the assumption that a new --tla-compat flag will be added in a follow-up PR).
  • --nodejs-compat: Handle Node.js instantiation when no fetch global is present

The above flags are only compatible with instance output and will throw otherwise.

This PR also parallelizes the core Wasm instantiations, and converts the instantiation API from instantiateCore(path, imports) to compileCore(path) to enable this parallelization.

In addition a new --map is added with support for custom import specifier mappings that can apply to both output modes. For example - --map "imports=./imports.js" allows specifying that the imports should resolve to a relative JS specifier.

For testing, the first line comment of the JS file starting with // Flags: is used to pass these output options flags, with most tests now including // Flags: --instantiation for backwards-compat.

Outputs a direct ES module from the JS host generator by default
that can be directly imported in JS environments.

For the custom instantiation API that was the previous output, a new
`--instantiation` flag is provided.

Instance-level customization flags are also added including:
* `--compat`: Enables compatibility mode, based on the assumption
  that a new --tla-compat flag will be added in a future PR.
* `--nodejs-compat`: Handle Node.js instantiation when no fetch global
  is present

The above flags are only compatible with instance output and will throw
otherwise.

In addition a new `--map` is added with support for custom import
specifier mappings that can apply to both output modes.
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me!

Could you expand on the longer-term story you see fo the --instantiation/etc flags? My experience in wasm-bindgen is that the flags for "what js environment are you targeting" can get out of hand quite quickly and become unwieldy to manage internally. The solution settled on over there was an all-encompassing --target $foo flag where there was no other need to have lots of other various flags.

Do you expect that the set of flags here will continue to grow? Are there lots of other targets that would grow custom flags that could have odd interactions with one another? Or perhaps would a --target flag make sense here as well?

crates/gen-host-js/tests/runtime.rs Outdated Show resolved Hide resolved
tests/runtime/flavorful/host.ts Show resolved Hide resolved
tests/runtime/flavorful/host.ts Show resolved Hide resolved
crates/gen-host-js/src/lib.rs Outdated Show resolved Hide resolved
crates/gen-host-js/src/lib.rs Outdated Show resolved Hide resolved
crates/gen-host-js/src/lib.rs Outdated Show resolved Hide resolved
crates/gen-host-js/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +967 to +969
if self.gen.opts.instantiation {
self.src.js("{}");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I would ideally prefer to have sort of a mish-mash sprinkling of if self.gen.opts.instantiation throughout since I feel that makes code difficult to read and change in the future. Would it be possible to instead change the return value of this function to something that works in both --instantiation and not modes? For example the exports here could return a form of HashMap<???, String> or something and then the caller would, once, check instantiation and create an export object or create a bunch of exported functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By switching to namespaces there is now much more consistent handling between the modes. Overall I think this gating is a semantically important one as it represents the difference between exporting a namespace and a top-level declaration.

crates/gen-host-js/src/lib.rs Outdated Show resolved Hide resolved
crates/gen-host-js/src/lib.rs Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

In terms of the compat / target flags, one thing I don't like about versioning targets is it puts too much responsibility on the user to know their exact platform support, whereas having good defaults and some hints / options can get you 99% of the way.

I guess the model I'm thinking of is that the default output works in most environments, then by adding --compat you get something that works everywhere, or by digging into the specific --x-compat flags you can work out your own kind of compat matrix as necessary.

The features we're branching on here are currently really just:

  • Top-level await support (~90% of browsers)
  • fetch support (basically everywhere except Node.js)
  • Node.js support (using readfile as a replacement for fetch)

That's really it as far as the current status is.

In future we might have import reflection to import the core modules instead of compileStreaming, but apart from that I think most other things would be at the level of core Wasm itself. Eg if we want to lean more heavily on new core Wasm features while still emitting legacy core modules and branching on that. But in theory even something like that could be captured by the current --compat system?

I'm not sure though, I may be going down that same path of progressive complexity here too, so would be interested your thoughts further on this and what you would have in mind for the above in a target-based system.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds reasonable to me, I'm happy to go with these flags for now and see how it ends up.

As part of this PR or as a follow up, would you be up for plumbing the new JS options into checkboxes and such for the demo page? The Wasmtime output has an example of this and I think it might be neat to get a demo online of the different outputs.

@guybedford
Copy link
Contributor Author

I've added the demo UI options for "Compat" and "Custom Instantiation". Will merge when CI passes.

@guybedford guybedford merged commit f22e230 into bytecodealliance:main Oct 28, 2022
@guybedford guybedford deleted the js-instance branch October 28, 2022 23:31
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

Successfully merging this pull request may close these issues.

None yet

2 participants