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.

Sign up for GitHub

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

Jump to bottom

RFC: Add support for local JS snippets in wasm-bindgen #6

Merged
merged 10 commits into from
Mar 5, 2019
Merged

RFC: Add support for local JS snippets in wasm-bindgen #6

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1c57ae8
RFC: Add support for local JS snippets in wasm-bindgen
alexcrichton Jan 8, 2019
9fe2a6f
Rename to final location if merged
alexcrichton Jan 8, 2019
51c09bf
Updates from comments
alexcrichton Jan 10, 2019
1c3e9ff
More minor updates
alexcrichton Jan 10, 2019
b83aabb
Reword output changes
alexcrichton Jan 10, 2019
6e6eb0a
Updates and comments
alexcrichton Jan 14, 2019
e6d588a
Fix some typos
alexcrichton Jan 15, 2019
4d990bb
Expand function_table a bit
alexcrichton Jan 15, 2019
32952b5
Explicitly allow loading from the build directory
alexcrichton Jan 28, 2019
02c4e1e
Add an inline_js attribute for procedural macros
alexcrichton Jan 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
280 changes: 280 additions & 0 deletions text/006-local-js-dependencies.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,280 @@
- Start Date: 2018-01-08
- RFC PR: (leave this empty)
- Tracking Issue: (leave this empty)

# Summary
[summary]: #summary

Add the ability for `#[wasm_bindgen]` to process, load, and handle dependencies
on local JS files.

* A new attribute for this will be added:
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved

```rust
#[wasm_bindgen(file = "foo.js")]
extern "C" {
// ...
}
```

* The `--browser` flag is repurposed to generate an ES module for the browser
and `--no-modules` is deprecated in favor of this flag.

* The `--nodejs` will not immediately support local JS snippets, but will do so
in the future.

# Motivation
[motivation]: #motivation

The goal of `wasm-bindgen` is to enable easy interoperation between Rust and JS.
While it's very easy to write custom Rust code, it's actually pretty difficult
to write custom JS and hook it up with `#[wasm_bindgen]` (see
[rustwasm/wasm-bindgen#224][issue]). The `#[wasm_bindgen]`
attribute currently only supports importing functions from ES modules, but even
then the support is limited and simply assumes that the ES module string exists
in the final application build step.

[issue]: https://github.com/rustwasm/wasm-bindgen/issues/224

Currently there is no composable way for a crate to have some auxiliary JS that
it is built with which ends up seamlessly being included into a final built
application. For example the `rand` crate can't easily include local JS (perhaps
to detect what API for randomness it's supposed to use) without imposing strong
requirements on the final artifact.

Ergonomically support imports from custom JS files also looks to be required by
frameworks like `stdweb` to build a macro like `js!`. This involves generating
snippets of JS at compile time which need to be included into the final bundle,
which is intended to be powered by this new attribute.

# Stakeholders
[stakeholders]: #stakeholders

Some major stakeholders in this RFC are:

* Users of `#[wasm_bindgen]`
* Crate authors wishing to add wasm support to their crate.
* The `stdweb` authors
* Bundler (webpack) and `wasm-bindgen` integration folks.

Most of the various folks here will be cc'd onto the RFC, and reaching out to
more is always welcome!

# Detailed Explanation
[detailed-explanation]: #detailed-explanation

This proposal involves a number of moving pieces, all of which are intended to
work in concert to provide a streamlined story to including local JS files into
a final `#[wasm_bindgen]` artifact. We'll take a look at each piece at a time
here.

### New Syntactical Features

The most user-facing change proposed here is the addition of a new attribute
inside of `#[wasm_bindgen]`, the `file` attribute. This configured like so:

```rust
#[wasm_bindgen(file = "foo.js")]
extern "C" {
// ... definitions
}
```

This declaration says that the block of functions and types and such are all
imported from the `foo.js` file. The `foo.js` file is resolved relative to the
crate root (where this is relative to is also discussed in the drawbacks section
below). For example a procedural macro will simply `File::open` (morally)
with the path provided to the attribute.

The `file` attribute is mutually exclusive with the `module` attribute. Only one
can be specified.

### Format of imported JS

All imported JS is required to written with ES module syntax. Initially the JS
must be hand-written and cannot be postprocessed. For example the JS cannot be
written with TypeScript, nor can it be compiled by Babel or similar.

As an example, a library may contain:

```rust
// src/lib.rs
#[wasm_bindgen(file = "js/foo.js")]
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
extern "C" {
fn call_js();
}
```

accompanied with:

```js
// js/foo.js

export function call_js() {
// ...
}
```

Note that `js/foo.js` uses ES module syntax to export the function `call_js`.
When `call_js` is called from Rust it will call the `call_js` function in
`foo.js`.

### Propagation Through Dependencies

The purpose of the `file` attribute is to work seamlessly with dependencies.
When building a project with `#[wasm_bindgen]` you shouldn't be required to know
whether your dependencies are using local JS snippets or not!

The `#[wasm_bindgen]` macro, at compile time, will read the contents of the file
provided, if any. This file will be serialized into the wasm-bindgen custom
sections in a wasm-bindgen specific format. The final wasm artifact produced by
rustc will contain all referenced JS file contents in its custom sections.

The `wasm-bindgen` CLI tool will extract all this JS and write it out to the
filesystem. The wasm file (or the wasm-bindgen-generated shim JS file) emitted
will import all the emitted JS files with relative imports.

### Updating `wasm-bindgen` output modes

The `wasm-bindgen` has a few modes of output generation today. This PR
proposes repurposing the existing `--browser` flag, deprecating the
`--no-modules` flag, and canonicalizing the output in three modes. All
modes will operate as follows:

* **Default** - by default `wasm-bindgen` emits output that assumes the wasm
module itself is an ES module. This will naturally work with custom JS
snippets that are themselves ES modules, as they'll just be more modules in
the graph all found in the local output directory.

* **`--no-modules`** - the `--no-modules` flag to `wasm-bindgen` is incompatible
Copy link

Choose a reason for hiding this comment

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

I think we need to support ES6 parsing + concatenation to support this use case, and I think it's worth it to do so.

Perhaps not necessary in the MVP, but shortly after the MVP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's true yeah that this can be supported! I ended up realizing that a bit later when writing this and didn't come back to fully update this section.

I'm not certain though that we want to continue to support it even if we add an ES6 parser. I agree that technically we'll be empowered to support it with such a parser, but it's becoming less clear to me at least what the main use case is. It seems like this is largely mostly used for demo purposes which almost always can assume a newer browser anyway. Apart from that if you want browser compatibility then it seems like you almost for sure want a bundler and want to avoid --no-modules (as the module format is likely the least of the concerns at that point, e.g. TextEncoder)

Overall this may be a case where we just need to put more thought into wasm-bindgen's output format. I find it unfortunate that we have to choose from so many options, it feels like we're just inheriting a lot of issues with the existing JS ecosystem and having to deal with them all at once...

Copy link

@Pauan Pauan Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the primary use case of --no-modules is for people who want a fast lightweight solution without needing to deal with npm or Webpack or Node.

In other words, people with Rust experience who want to avoid the JS ecosystem as much as possible.

There is some merit to that, since npm and Webpack are rather slow (and have a lot of dependencies).

I don't have particularly strong opinions on it, I've comfortably used Webpack for many years (it is slow though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I'm thinking we should continue this conversation below

with ES modules because it's intended to be included via a `<script>` tag
which is not a module. This mode, like today, will fail to work if upstream
crates contain local JS snippets. As a result, the `--no-modules` flag will
essentially be deprecated as a result of this change.

* **`--nodejs`** - this flag to `wasm-bindgen` indicates that the output should
be tailored for Node.js, notably using CommonJS module conventions. This mode
will, in the immediate term, fail if the crate graph includes any local JS
snippets. This failure mode is intended to be a temporary measure. Eventually
it should be relatively trivial with a JS parser in Rust to rewrite ES syntax
of locally imported JS modules into CommonJS syntax.

* **`--browser`** - currently this flag is the same as the default output mode
except that the output is tailored slightly for a browser environment (such as
assuming that `TextEncoder` is ambiently available). This RFC proposes
repurposing this flag (breaking it) to instead generate an ES module natively
loadable inside the web browser, but otherwise having a similar interface to
`--no-modules` today, detailed below.

In summary, the three modes of output for `wasm-bindgen` will be:

* Bundler-oriented (no flags passed) intended for consumption by bundlers like
Webpack which consider the wasm module a full-fledged ES module.

* Node.js-oriented (`--nodejs`) intended for consumption only in Node.js itself.

* Browser-oriented without a bundler (`--browser`) intended for consumption in
any web browser supporting JS ES modules that also supports wasm. This mode is
explicitly not intended to be usable with bundlers like webpack.

The `--no-modules` flag doesn't really fit any more as the `--browser` use case
Copy link

Choose a reason for hiding this comment

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

This sounds rather strange: old browsers don't support ES6 modules, so we still need --no-modules for the foreseeable future. In addition, Node will be adding in support for ES6 modules.

So it seems to me that the distinction isn't about "browser" vs "Node", it's about "no modules" vs "ES6 modules" vs "CommonJS modules".

It should absolutely be possible to use ES6 modules with Node, and to use "no modules" with either Node or the browsers.

So could you explain more about the motivations behind removing "no modules" and assuming that browsers have ES6 support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm personally more wary on this, expecially with Node and ES6 modules. AFAIK everyone agrees that ES6 modules should come to node but there hasn't been any consensus or real timeline to getting it added. The current mjs implementation feels pretty bad to use and definitely seems like something that we shouldn't be targeting.

We definitely have two axes here, though. One is Node vs browser environments (like where you get TextEncoder from). The other is the module system used (ES, none, CommonJS, etc). The "ideal" of "ES6 everywhere" definitely can't be a reality today largely because of Node I think. Apart from that we're sort of just doing the best we can.

I'm hoping that we can pick a somewhat opinionated set of defaults and support. For example no modules on Node while possible I don't think would have many users. Similar for CommonJS in browsers I'm not really sure what the use case. Along these lines it seems that the --no-modules use case for browsers is becoming less useful and I'm not entirely certain where it'd be used. (I mentioned above as well, but for older browser compat it seems like you'd almost always use a bundler)

This definitely isn't a strong motivation for removing --no-modules though. I mentioned above too, but we can definitely keep supporting it. That may be the best for now while we continue to wait for the module story in JS to settle...

Copy link

Choose a reason for hiding this comment

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

I see, so your perspective is basically "ES6 modules + bundler" is the replacement for --no-modules?

That sounds reasonable to me, though I know some people won't like that.

I agree that ES6 modules aren't ready for Node (and won't be anytime soon), I just don't want us to be locked into that sort of decision: we should be able to support ES6 modules in Node eventually (even if not right now).

So, as you say, there are really two different axes here: runtime and module.

Perhaps you're right and we can condense that down to one axis.

Of course if we wanted to be really reductionistic we could only support one output format and rely upon bundlers like Webpack to handle all the intricacies of browser vs Node (some people do indeed use bundlers even on Node).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I'm thinking we should continue this conversation below

is intended to subsume that. Note that the this RFC proposes only having the
bundler-oriented and browser-oriented modes supporting local JS snippets for
now, while paving a way forward to eventually support local JS snippets in
Node.js. The `--no-modules` could eventually also be supported in the same
manner as Node.js is (once we're parsing the JS file and rewriting the exports),
but it's proposed here to generally move away from `--no-modules` towards
`--browser`.

For some more detail about the `--browser` output, it's intended to look from
the outside like `--no-modules` does today. When using `--browser` a single
`wasm_bindgen` function will be exported from the module. This function takes
either a path to the wasm file or the `WebAssembly.Module` itself, and then it
returns a promise which resolves to a JS object that has the full wasm-bindgen
interface on it.

### JS files depending on other JS files

One tricky point about this RFC is when a local JS snippet depends on other JS
files. For example your JS might look like:

```js
// js/foo.js

import { foo } from '@some/npm-package';
import { bar } from './bar.js'

// ...
```

As designed above, these imports would not work. It's intended that we
Copy link

Choose a reason for hiding this comment

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

I had covered this extensively several months ago:

rustwasm/team#92 (comment)
rustwasm/team#36 (comment)
rustwasm/team#36 (comment)

You don't need to read them, I'll summarize it here.

Basically, wasm-bindgen would look through the crate and find all files that end in .js. Then it would put those files into the custom section (with the file path relative to the crate).

When generating the output, it would then recreate the same folder structure and place the .js files into the output folder.

This has a few major advantages:

  • wasm-bindgen doesn't need to parse .js files at all. It only needs to read the file and keep track of the relative file path.

  • It fully supports JS files importing other JS files, without any effort from wasm-bindgen (we get it "for free").

  • It fully supports dynamic import().

    The only other way to support dynamic import() is to explicitly list the .js files in metadata (such as in package.json or Cargo.toml).

    But with this approach, everything "Just Works", without needing to specify any metadata.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A compelling alternative! There's a few things though that make me wary of this, and I think overall I'm not personally convinced we should take the "search the whole crate" strategy:

  • I think we'd still need a parser to support --no-modules and --nodejs, so while it may mean that some use cases don't need a parser in general I think we'd still need it.
  • Incremental builds in Rust I don't think would be supported well. Cargo doesn't really have the ability for a crate to say "please rebuild when a file is added". For Rust you'd have to edit a preexisting file to include a module for example. To that end I'm don't think we could correctly rebuild a crate if we automatically included all JS
  • I'd be worried about namespacing issues here as well. For example if two crates have /js/foo.js, then we'd have to disambiguate that somehow. To solve this we'd need to emit files like /$hash/js/foo.js and make wasm import from there, but it means that any absolute imports in the local JS files couldn't be written down. I believe all relative imports would work, however!
  • To actually do this in the macro I think we'd need to do something like add: 
    wasm_bindgen::include_local_js!();
    where we need an explicit include to have a point in the crate where JS is included. That feels somewhat unnatural wrt the current #[wasm_bingen] sprinkled around. I'm not sure we could do something like "inject it into the first #[wasm_bindgen].

Ideally though we could end up on a design which doesn't block this from ever happening. It seems reasonable to have as a configuration option at least!

Copy link

Choose a reason for hiding this comment

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

I think we'd still need a parser to support --no-modules and --nodejs, so while it may mean that some use cases don't need a parser in general I think we'd still need it.

That is true, that negates one of the advantages. Though I think the other advantages are still really good!

Incremental builds in Rust I don't think would be supported well.

Oh, I hadn't considered that. That's a good point. If users manually specified the .js files in package.json, would those changes be picked up? Or would it only be changes in Cargo.toml that are picked up?

I'd be worried about namespacing issues here as well.

I had put a lot of thought into that (several months ago), and I had a few solutions for that. They basically all involved putting each crate into a different subfolder, thus avoiding the namespace issue entirely. So, similar to your /$hash/js/foo.js idea.

As for absolute imports, that's not a problem: ES6 modules and bundlers already don't support absolute imports (or at least, support them very poorly). And in order to publish your packages, you can't use absolute imports anyways. So there's no issue with mandating file-relative or crate-relative imports.

To actually do this in the macro I think we'd need to do something like [...]

I hadn't thought too much about the actual implementation in wasm-bindgen. It does seem rather tricky, since you can't have top-level function calls.

And given that it has issues with incremental compilation, maybe we should consider other alternatives instead.

The only alternative I know of is to manually list .js files in package.json or Cargo.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that we'll likely end up in a position where you have to list the JS files to be included, but I like the idea of automatically namespacing everything and preserving file structure to support things like dynamic import. I don't think we'd want to disallow static absolute paths if we force Rust to use absolute paths for now, but with a JS parser we could rewrite to absolute paths (or at least with our hash prefix) and force dynamic imports to never use absolute paths.

I do really like the UI though of "just pull in all JS", and I'm wondering if we should add a feature to Cargo to support this. It wouldn't be too too hard to say "Cargo I depend on this entire directory, anything added or changed here please rerun me" and that could be used to easily include a js folder in a wasm-bindgen application

Copy link

Choose a reason for hiding this comment

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

Perhaps we're using a different meaning for "absolute path"?

To me, "absolute path" means an absolute path on the user's harddrive, which isn't acceptable since the filepath is outside of the crate (and thus it will break for anybody other than that specific user). Perhaps you mean something different?

Absolute paths are basically never used (for the above reason), instead relative paths are the norm, so what do you mean by "we could rewrite to absolute paths"?

force dynamic imports to never use absolute paths

How can that be enforced? Dynamic imports are resolved at runtime, so it's impossible (in the general case) to figure out what file they are importing.

That's why the "slurp up all the .js files" (or "list all the .js files in package.json") strategy is necessary.

It wouldn't be too too hard to say "Cargo I depend on this entire directory, anything added or changed here please rerun me" and that could be used to easily include a js folder in a wasm-bindgen application

That sounds like it could be a useful feature even outside of wasm-bindgen!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry, yes we are using the same term for different things! This is about local files importing other local files, so I'd imagine that we do something like this:

import { foo } from './other-local.js'; // allowed, it's relative
import { bar } from '../other-local.js'; // allowed, it's relative
import { baz } from '/other-local.js'; // allowed, but we have to rewrite this import


import('./x.js'); // we'll make sure this works
import('/x.js'); // this will always fail at runtime, no way we can make this work

In Rust code we'd have:

#[wasm_bindgen(module = "./foo.js")] // disallowed, don't know how to implement today
extern {}

#[wasm_bindgen(module = "../foo.js")] // disallowed, don't know how to implement today
extern {}

#[wasm_bindgen(module = "/foo.js")] // allowed, rooted-at-the-crate-root absolute path
extern {}

By "absolute" I mean "absolute but rooted at the crate root".

I think how this all works though is somewhat orthogonal to whether we slurp up JS files or not. On one axis we have what all the paths are, and on another axis we have "do you list the files to include or do we automatically include?". I think on "how do handle paths" it's relatively unambiguous about what to do perhaps modulo minor tweaks. For "do we slurp up JS or not?" I'm proposing that we don't do it yet, but we leave ourselves an option in the future for doing so. (but also not doing a list-some-files-to-include for now)

Copy link

Choose a reason for hiding this comment

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

I had assumed that / paths simply wouldn't be allowed in .js files (only ./ and ../ would be allowed).

I think that it would be confusing and inconsistent to have different behavior with import statements and import() expression.

Also, using / in ES6 import paths is basically never done (and with browsers it means "relative to the web server root", not "relative to the crate root"), another reason to not use / in .js files.

By "absolute" I mean "absolute but rooted at the crate root".

Okay, glad we're on the same page now. I would personally call those "crate paths" or something. Maybe we should bikeshed the name a little bit, since it will show up in the docs?

For "do we slurp up JS or not?" I'm proposing that we don't do it yet, but we leave ourselves an option in the future for doing so. (but also not doing a list-some-files-to-include for now)

Sure, that's fine. It's only needed for dynamic import(), which we will need to support eventually, but not in the MVP.

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm ok sorry, I may not be being clear. I'll try to respond to those points:

  • We will eventually allow absolute and relative paths in #[wasm_bindgen(module = "path")]
    • Absolute paths are rooted at the crate root
    • Relative paths cannot be implemented with stable Rust today. We'll enable this in the future.
  • import { ... } will, like #[wasm_bindgen], allow all three paths
    • The RFC as proposed right now, however, says we'll be doing this at a future date and won't implement local-js-depending-on-local-js just yet. It just won't work and will fail with a weird error at bundler/load time
    • Eventually, absolute paths are intended to work and will be rooted at the crate root
    • Eventually, relative paths will also work as we'll have a way to ship multiple files preserving filesystem structure (bit it declarative as to which files are shipped or inferred)
  • import(...) will disallow absolute paths.
    • Like import { ... } this won't work to start off with at all and will fail with weird errors
    • Like import { ... } relative paths will work by shipping multiple files
    • Absolute paths won't ever work because we'll have to rewrite the filesystem hierarchy to handle namespacing conflicts.

Sorry this is so confusing, I'm trying to both lay out a plan for what we should do now in this RFC with wasm-bindgen while also handle all of what you're saying and plan for the eventual support for all these features. I think that a lot is getting lost in the middle as it's not clear what phase of development is being discussed.

For absolute paths and/or crate paths, we may want to figure out a different syntax if it's very rarely used in the rest of the JS ecosystem. We could try out like $crate/... perhaps? My point though about allowing it in JS files (not in import(..), only in static imports) is that we're requiring it in Rust code (for now) so it seems natural to write in JS code too.

Copy link

@Pauan Pauan Jan 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We will eventually allow absolute and relative paths in #[wasm_bindgen(module = "path")]

We are in agreement here.

import { ... } will, like #[wasm_bindgen], allow all three paths

We are in disagreement here. I think .js files should never support absolute/crate paths, only relative paths.

I think the inconsistency between import statements and the import() expression isn't good. The spec authors put a lot of effort into making import() equivalent to import, and for good reason.

And I think that we shouldn't break the already existing conventions for .js files (which are exclusively relative paths).

My point though about allowing it in JS files (not in import(..), only in static imports) is that we're requiring it in Rust code (for now) so it seems natural to write in JS code too.

I understand and empathize with that viewpoint, but I don't agree with it (for the above reasons). In different circumstances, I would agree with it.

I think that .js files are in a sense a "different world" from .rs files, so using different path systems (crate vs relative) is okay.

Especially because relative paths in .rs files won't work anytime soon (and might never work), so there's already an inconsistency between .rs and .js.

I agree that it certainly would be nice if we could make the path system consistent between Rust and JS, but given the existence of import() and the limitations of proc-macro, it doesn't seem possible.

explicitly say this is an initial limitation of this design. We won't support
imports between JS snippets just yet, but we should eventually be able to do so.

In the long run to support `--nodejs` we'll need some level of ES module parser
for JS. Once we can parse the imports themselves it would be relatively
straightforward for `#[wasm_bindgen]`, during expansion, to load transitively
included files. For example in the file above we'd include `./bar.js` into the
wasm custom section. In this future world we'd just rewrite `./bar.js` (if
necessary) when the final output artifact is emitted. Additionally with NPM
package support in `wasm-pack` and `wasm-bindgen` (a future goal) we could
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
automatically add entries to `package.json` (or validate they're already
present) based on the imports found.

# Drawbacks
[drawbacks]: #drawbacks

* The initial RFC is fairly conservative. It doesn't work with `--nodejs` out of
the gate nor `--no-modules`. Additionally it doesn't support JS snippets
importing other JS initially. Note that all of these are intended to be
supported in the future, it's just thought that it may take more design than
we need at the get-go for now.

* JS snippets must be written in vanilla ES module JS syntax. Common
preprocessors like TypeScript can't be used. It's unclear how such
preprocessed JS would be imported. It's hoped that JS snippets are small
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
enough that this isn't too much of a problem. Larger JS snippets can always be
extracted to an NPM package and postprocessed there.

* The relatively popular `--no-modules` flag is proposed to be deprecated in
favor of a `--browser` flag, which itself will have a breaking change relative
to today. It's thought though that `--browser` is only very rarely used so is
safe to break, and it's also thought that we'll want to avoid breaking
Copy link
Member

Choose a reason for hiding this comment

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

I do have concerns about changing the meaning of existing flags and whether or not we would need to do a breaking bump. I am not sure that this is "safe" (I have no idea and no data and I don't think you have any more data than I have!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry meant to respond to this earlier, but you're spot on that I have very little data about this :)

I'm curious to hear what others think about this, although I suspect you're right and that we'll need to make new flags instead of trying to repurpose existing flags.

Copy link

Choose a reason for hiding this comment

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

This PR seems primarily focused on module resolution/loaders, so perhaps their names could reflect this better? For example, we could have something like --module-resolution={none,node,browser}:

  • none is the default suggested in this PR and means that either no modules need to be loaded or some bundler is expected
  • node and browser replace the flags suggested in this PR

There is some precedent in TypeScript too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm the module resolution aspect looks like it's more worried about how to resolve the paths in file dependencies (aka the "foo" in import x from "foo"). For us though it's more about what to generate at all, e.g. import or require or "somehow nothing"

Copy link

Choose a reason for hiding this comment

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

You're right, the concepts are fairly related though (especially with mjs in Node). Maybe module-loader or similar instead of module-resolution?

browser and node seem a bit general because they could imply whether web APIs etc. are available as well. Although that might be ok if the flags will eventually be used beyond module concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're similar yeah, but the flags proposed here are intended to capture the use cases that basically everyone seems to fall into, which is "browsers with bundlers", "browsers without bundlers", and "node"

`--no-modules` as-is today.

* JS files are imported relative to the crate root rather than the file doing
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
the importing, unlike `mod` statements in Rust. It's thought that we can't get
file-relative imports working with stable `proc_macro` APIs today, but if the
implementation can manage to finesse it this RFC would propose instead making
file imports relative to the current file instead of crate root.

* Local JS snippets are required to be written in ES module syntax. This may be
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
a somewhat opinionated stance, but it's intended to make it easier to add
future features to `wasm-bindgen` while continuing to work with JS.

# Rationale and Alternatives
[alternatives]: #rationale-and-alternatives

Currently there aren't any competing designs solving this problem, so there
alexcrichton marked this conversation as resolved.
Show resolved Hide resolved
aren't many known major alternatives. There's a number of alternatives to
various smaller decisions in this RFC, but it's hoped that the various
alternatives are listed or discussed inlined.

The major rationale for this RFC is empowering this use case of "seamless local
JS snippets" **at all**. The RFC proposes to start out with only including files
as opposed to small JS snippets themselves (like `stdweb`'s own `js!` macro)
because files are structured as ES modules which is what `#[wasm_bindgen]`
already supports well and will also eventually have support natively in
WebAssembly itself. While `#[wasm_bindgen]` may one day have a `js!`-like macro
built-in, it's hoped that we can start out with a purely library-based solution
like `stdweb`, iterate on it, and consider this question later.

# Unresolved Questions
[unresolved]: #unresolved-questions

- Is it necessary to support `--nodejs` initially?

- Is it necessary to support local JS imports in local JS snippets initially?

- Are there known parsers of JS ES modules today? Are we forced to include a
Copy link

Choose a reason for hiding this comment

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

If we simply want to support "JS files importing other JS files", I think a very minimal parser should be fine.

If we want to support ES6 module concatenation, then we probably need a full parser. But as I said above, I think we should try to use existing solutions rather than making our own (unless there's a good reason to make our own).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One aspect I'm worried about is that we have to also parse global declarations like let x = 3; which I fear will pull in a full JS parser to handle that.

Copy link

Choose a reason for hiding this comment

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

If we want to support concatenation built-in to wasm-bindgen, then yeah we need a full parser. But if it's just parsing import stuff to find other .js files to import, that's a different story.

Actually, come to think of it, how will incremental building work with .js files importing other .js files? If a user edits a .js file and adds a new import, will that change be picked up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My hope is to use include_str! or similar to inject a dependency via rustc on the actual file itself, so Cargo will rerun the build of a crate if the .js file is edited. For import it sort of naturally falls out from the rest of this RFC. It's either supported by parsing the file (so we'll re-parse and figure it out), supported by pulling in everything (but has the drawback that we can't handle newly added files), or not supported right now (which is what this RFC currently proposes)

Copy link

@Pauan Pauan Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I see! So I guess that would also work for package.json? So if wasm-bindgen uses include_str! for package.json, then if somebody edits package.json to add a new .js file, the change will be picked up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK we don't actually have much reason to worry about package.json in wasm-bindgen, only wasm-pack. In wasm-pack everything is unconditionally done all the time (as it's so fast), but if we were to need to parse things in wasm-bindgen this'd be how we do it!

Copy link

Choose a reason for hiding this comment

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

So how is that intended to work (especially with transitive crates)? wasm-pack can't access transitive crates, right?

I know that's going a bit beyond this RFC, but since it relates to dynamic import() I think it's somewhat relevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm I may be missing some context to the question, can you be a bit more specific about what you're curious will work?

To try to answer ahead of time, though, you're right that wasm-pack (and the wasm-bindgen CLI) don't have access to the particulars of the crate graph. The #[wasm_bindgen] macro does, however, have access! To that end the #[wasm_bindgen] macro can encode information into the custom section (which gets concatenated) about how to find JS files. It can either serialize them directly into the custom section or leave filesystem paths in the custom section which immediately get removed during wasm-bindgen-the-CLI.

In that sense the outer tools don't have direct access to transitive crates, but the macro can encode and provide all the information they should need to operate.

Copy link

Choose a reason for hiding this comment

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

To try to answer ahead of time, though, you're right that wasm-pack (and the wasm-bindgen CLI) don't have access to the particulars of the crate graph.

That's what I'm concerned about. In order to make things work correctly, it is necessary for package.json to work with transitive crates. It's not that important for this RFC, but it's important in general.

So if wasm-bindgen doesn't include the package.json in the custom section, how will wasm-pack be able to handle package.json in transitive crates?

Maybe we should move this conversation to another issue, since it seems to be getting quite off-topic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh sorry my point is that I agree that transitive crates should always work, and the #[wasm_bindgen] macro is how we'll solve that. If we need to ship package.json (or anything else) from dependencies to the final artifact, we'll do it in the #[wasm_bindgen] macro

(moving to a separate issue is also fine by me

full JS parser or can we have a minimal one which only deals with ES syntax?