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

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

Merged
merged 10 commits into from
Mar 5, 2019

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Jan 8, 2019

@fitzgen
Copy link
Member

fitzgen commented Jan 8, 2019

Overall Impression

I think we are on the right track here.

I agree that having the #[wasm_bindgen] proc-macro collect local JS snippets into custom sections that are then processed by the wasm-bindgen CLI is the correct approach.

Modules vs. Environments

I think it is worth considering and being more explicit about how this RFC is moving us further away from talking about targeting different module systems, and closer to targeting JS environments. That is, we are not talking about emitting ES modules vs no modules anymore, we are talking about emitting node.js-compatible vs browser-compatible code (which involves more than only module format). It also means that in cases where an environment supports multiple module systems, or the module system is optional (browsers support es modules and also no modules) wasm-bindgen will choose what module system it thinks is best as long as it is compatible with that environment. This seemingly small paradigm change is going to have pretty large effects as it ripples out.

If I understand correctly, this is what we will end up with:

Target Environment Module Format User Experience How are Local JS Snippets Loaded?
Node.js without bundler Common.js require() the main JS glue file Main JS glue file require()s crates' local JS snippets.
Web without bundler ES Modules <script> pointing to main JS glue file import statements cause additional network requests for crates' local snippets.
Web with bundler ES Modules <script> pointing to main JS glue file Bundler links crates' local snippets into main JS glue file. No additional network requests except for the wasm module itself.

It is notable that browser with and without bundler is almost the same as far as wasm-bindgen is concerned: the only difference is that if we assume a bundler, we can rely on the bundler polyfilling wasm-as-ES-module for us.

Other Assets?

What if a crate wants to include CSS? Images? HTML snippets?

Suggested Potential Alternatives

Concatenate Local JS Snippets

We could take all the local JS files and concatenate them, instead of requiring they be written as ES modules. This allows us to kick the problem of module format down the road to crate authors, but has the following downsides:

  • Concatenating JS files can lead to either potential namespacing conflicts, module format disagreements (crate A uses es modules in its local snippet, crate B uses common js in its local snippet, the concatenated js snippet now uses two module formats), or we would have to implement a whole JS linker/bundler in wasm-bindgen.

  • If a crate uses local JS snippets in a particular module format, then consumers of that crate would be forced to support that module format. They can maybe use a bundler or something to help get support for many different module formats, but the burden is on them. On the other hand, by requiring local snippets be written as ES modules, we are guaranteeing the most portable, future-looking module format, and it is easy to translate into other module formats inside wasm-bindgen without implementing a whole JS linker/bundler.

It is worth calling out that we don't want to implement a whole JS linker/bundler, as it seems pretty easy to accidentally go down that route if we aren't careful.

Leave the Local JS Snippets in the Wasm Custom Sections

And then unpack them at runtime (with eval or <script> tags). This means that there are less files being wrangled and no extra HTTP requests as if you were using a bundler even when you aren't, but runs afoul of strict CSP settings.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Couple nitpicks

* 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.

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"

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"

text/006-local-js-dependencies.md Show resolved Hide resolved
@koute
Copy link

koute commented Jan 9, 2019

So far this looks like a good starting point!

I have a few comments/questions as far as creating an actual js! macro implementation on top of this:

  1. How would such a JS snippet go about accessing the module's memory? (For passing through types which are currently not natively supported by the WASM ABI.)
  2. How would such a JS snippet access the WASM module's table? (For calling callbacks passed from Rust to JS.)
  3. The inability to depend on other JS files is very unfortunate. Could we perhaps extend the #[wasm_bindgen] macro itself so that we could specify on Rust side what we'd like to import? (For importing stdweb's runtime.) If not then even a very limited super-minimal parser for imports would be great to have, e.g. one which would only accept strictly formatted code and reject anything else.

To port stdweb to wasm-bindgen as-is with full functionality all three of these would have to be supported in some form.

Another thing to potentially think about is - do we actually want to emit each JS snippet in a separate file? That could technically result in hundreds of JS files being generated. Certainly until stdweb switches to js-sys/web-sys it would generate a lot of those (they're currently ~774 instances of js! if you grep stdweb's sources), and surely any bigger app could potentially use a lot of such snippets.

@alexcrichton
Copy link
Contributor Author

@fitzgen excellent points on the shift in paradigms, I'll definitely want to include that in the text. Also those are some solid alternatives, I'll be sure to include those too!

For other assets I'm not really sure how we'd handle that for now, I'm hoping that's largely up to bundlers still, but I can see how they might still be wanted in some cases. For now though I'll leave it as an unresolved question?


@koute thanks for taking a look over this! I'd forgotten about accessing memory/table of the wasm instance, as that seems pretty critical to support and definitely doesn't have any means for support in this proposal!

I'm not really sure of the best way to support importing the memory/table, but the best thing I can come up with is something like:

import { memory, table } from "@wasm-bindgen";

// ...

that'd have the following caveats:

  • We'd rewrite the string "@wasm-bindgen" to the actual wasm file that we generate for bundlers/browsers to consume.
  • The "@wasm-bindgen" module string would be sort of magical, as well as the names of "memory" and "table", which I'm not a huge fan of...
  • This would require rewriting imports from the very beginning which would be somewhat unfortunate :(
  • We'd have to change the module structure of the --browser use case to actually export everythign directly from the module, but all functions would be "inert" until a boot function was called and the returned promise resolves (sort of like what wasm2es6js does today). I think that may be a bit of a better design in general anyway though!

For importing JS files I think that changing the Rust side is definitely something we can do, but we may not want to do it long-term. Is importing other JS snippets critical for stdweb's use case? Honestly it's looking like we'll need to figure out the ES module parsing/rewriting from step 1, so we may just want to tackle this problem directly instead of postponing.

As for file sizes and number of files, I figure we can see how it plays out with bundlers! It may mean that the js! macro in stdweb wants to be structured a bit differently (place everything into one JS file?) or something like that, but I figure we can iterate over time. If we parse everything we may be able to automatically concatenate everything too.

@xtuc
Copy link
Member

xtuc commented Jan 10, 2019

I only did a quick look at it (I'm feeling way too jet-lagged currently, sorry), thanks for pushing this forward.

  • It's unclear to me if wasm-bindgen will emit the according module name in the Wasm module (ex (import "file.js" "module_identifier..."), we use this section to wire up dependecies during bundling (also that's the esm-integration proposal in the WebAssembly GC).

  • I think that commonjs would also work with Wasm in Webpack (needs confirmation), that would be required for most of JS packages on npm.

@fitzgen
Copy link
Member

fitzgen commented Jan 10, 2019

* It's unclear to me if wasm-bindgen will emit the according module name in the Wasm module (ex `(import "file.js" "module_identifier...")`, we use this section to wire up dependecies during bundling (also that's the esm-integration proposal in the WebAssembly GC).

Yes, wasm-bindgen would still emit the extern imports that would ultimately translate into imports in the wasm binary. I'd expect that the file = .. attribute would be reflected in the module used in the import. We should note this in the RFC.

@fitzgen
Copy link
Member

fitzgen commented Jan 10, 2019

@koute

How would such a JS snippet go about accessing the module's memory? (For passing through types which are currently not natively supported by the WASM ABI.)

@alexcrichton

I'm not really sure of the best way to support importing the memory/table, but the best thing I can come up with is something like:

import { memory, table } from "@wasm-bindgen";

// ...

An alternative design would be for the JS snippet's functions to take a reference to the wasm memory, and for the Rust to pass it along. Something like this:

// local-snippet.js

export function take_u8_slice(memory, ptr, len) {
    let slice = new UInt8Array(memory.arrayBuffer, ptr, len);
    // ...
}
// lib.rs

#[wasm_bindgen(file = "local-snippet.js")]
extern {
    fn take_u8_slice(memory: &JsValue, ptr: u32, len: u32);
}

#[wasm_bindgen]
pub fn call_local_snippet() {
    let vec = vec![0,1,2,3,4];
    let mem = wasm_bindgen::memory();
    take_u8_slice(&mem, vec.as_ptr() as usize as u32, vec.len() as u32);
}

This has the advantage of not requiring additional special imports or anything like that. And if you are implementing a custom ABI outside of the usual wasm-bindgen mechanisms, then your likely going to wrap up the raw FFI in helper functions anyways.

@alexcrichton
Copy link
Contributor Author

@fitzgen I like your suggestion of wasm_bindgen::memory(), that sounds like the best option for now!

I'll add to the RFC that we'll likely want to add a wasm_bindgen::function_table() to mirror wasm_bindgen::memory()

@Pauan
Copy link

Pauan commented Jan 11, 2019

(I'll be reviewing this soon, I've been busy with moving to another country)

text/006-local-js-dependencies.md Outdated Show resolved Hide resolved
currently only consumable by bundlers like Webpack, the default output cannot
be loaded in either a web browser or Node.js.

* **`--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

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

enough that we can get away with this, but feedback is always welcome on this
point!

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

// ...
```

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.

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

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.

text/006-local-js-dependencies.md Show resolved Hide resolved
text/006-local-js-dependencies.md Outdated Show resolved Hide resolved
text/006-local-js-dependencies.md Show resolved Hide resolved
text/006-local-js-dependencies.md Show resolved Hide resolved

- 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

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

- Are there known parsers of JS ES modules today? Are we forced to include a
full JS parser or can we have a minimal one which only deals with ES syntax?

- How would we handle other assets like CSS, HTML, or images that want to be
Copy link

Choose a reason for hiding this comment

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

I think this goes far beyond supporting .js files, since that question isn't even answered in the JS world!

The typical solution is to import the CSS/HTML/whatever normally, and then let the bundler (e.g. Webpack/Parcel) handle it.

That's not really a great solution, but it's the best solution found in the JS world.

We should revisit this question much later, after we have more experience with the system.

@Pauan
Copy link

Pauan commented Jan 12, 2019

P.S. I'm really glad to see progress on this. It's one of the few major things missing from wasm-bindgen.

@alexcrichton
Copy link
Contributor Author

Ok I've pushed an update now which drops the new file attribute and simply repurposes the existing module attribute (at @Pauan's suggestion). The tl;dr is:

  • Paths starting with /, ./, or ../ are considered to point to files. All other paths are passed through verbatim like today.
  • Paths are rooted at the crate root and relative to the current file. We don't know how to implement relative to the current file yet, so all paths will start with / and be relative to the crate root.

One thing I think is worth discussing more at this point is what to do about --no-modules. Supporting --no-modules with this proposal will require a JS parser to concatenate ES modules. That means in the near term --no-modules may stop working if dependencies start using module = "/foo.js". What do we do about this?

On one hand we can deprecate the --no-modules option and discourage use in the future. (that's what this RFC proposes). Alternatively we could simply consider it like --nodejs where it's "temporarily not well supported but coming soon". To try to break this down, at a technical level I think that --no-modules is used in a variety of scenarios today:

  • One is running in browsers without bundlers. This is supported in this RFC with --browser. The output of --browser may not be quite as "minimal" and/or efficient as --no-modules, but it's thought that if you're gunning for performance a bundler would be used instead.
  • Another known use case is dealing with "esoteric environments". For example audio worklets, web workers, CloudFlare workers, testing bleeding edge features, etc. These environments often don't have great integration with ES modules or something like that. Put more simply, JS isn't JS if you can't define a function, but it's still JS if you can't define a module.

All in all I'm not really sure what to do or honestly what sort of impact it will have. I feel like we should basically just "wait and see". In the near term we fundamentally can't support --no-modules, but in the future we'll support --nodejs and it'll naturally extend to supporting --no-modules as well. In the interim I think we can gather data and develop a gut feeling for whether we need to keep supporting --no-modules or not.

Copy link

@Pauan Pauan left a comment

Choose a reason for hiding this comment

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

Overall I really like this RFC! I think it's in a good spot.

There are some things that need to be worked out with regard to transitive dependencies and dynamic import() (e.g. package.json), however they don't block the RFC, since they can be worked out during the implementation.

As for the issue about handling "no modules", I agree with @alexcrichton that we should wait and see. It's okay for the MVP to not support --nodejs and --no-modules.


With regard to stdweb, I had an idea: what if stdweb took all of the js! snippets, concatenated them together, then put them into the wasm-bindgen custom section? It could do the same for the stdweb runtime.

That way stdweb doesn't need to create hundreds of files (in fact it doesn't need to create any files at all). What do you think @koute?

text/006-local-js-dependencies.md Outdated Show resolved Hide resolved
text/006-local-js-dependencies.md Outdated Show resolved Hide resolved
text/006-local-js-dependencies.md Outdated Show resolved Hide resolved
@fitzgen
Copy link
Member

fitzgen commented Jan 15, 2019

Quick question:

wasm_bindgen::function_table()

What exactly is this going to be? Is it going to be a wasm-bindgen intrinsic that is implemented in JS and returns the exported function table as a WebAssembly.Table JS object?

@alexcrichton
Copy link
Contributor Author

@fitzgen

Is it going to be a wasm-bindgen intrinsic that is implemented in JS and returns the exported function table as a WebAssembly.Table JS object?

Indeed!

@alexcrichton
Copy link
Contributor Author

The more I think about this RFC the more I feel that we need to have our ducks in a row before merging to ensure that we've got an implementation lined up for "compile ES modules to CommonJS" . That's such a critical component of the long-term viability of this feature that I want to make sure it doesn't fall by the wayside.

To that end I took a brief look at the ecosystem of JS parsers written in Rust, and what I found was:

  • esprit - unfortunately looks a bit old and non-maintained, so didn't dig in too closely
  • ratel - looked quite old on crates.io but the repository seems active. Due to the age on crates.io I didn't look too closely
  • swc_ecma_parser - looks maintained and have seen some news about it recently, but it requires nightly Rust
  • ressa - paired with the resw crate looked like a good candidate

I reached out to @FreeMasen (author of ressa and resw) and they sound like they're on board with being used by wasm-bindgen, so that seems like a solid way forward! I've done some initial testing of the parser/writer against spidermonkey tests and while not perfect it looks pretty solid already. I'm sure a little elbow grease can take it the rest of the way.

Before we officially commit to anything I'm curious if others know JS parsers that we should take a look at? Should I take a closer look at some of the crates above?

@Pauan
Copy link

Pauan commented Jan 15, 2019

swc_ecma_parser - looks maintained and have seen some news about it recently, but it requires nightly Rust

"feature-complete ES2019 parser" sounds really compelling. Can we investigate what it would take to get swc_ecma_parser working on Stable?

@Pauan
Copy link

Pauan commented Jan 15, 2019

ratel - looked quite old on crates.io but the repository seems active. Due to the age on crates.io I didn't look too closely

If the GitHub is active, maybe we just need to ping them to update crates.io?

@Pauan
Copy link

Pauan commented Jan 15, 2019

(To be clear, I have nothing against ressa/resw, I just want us to explore all our options)

fitzgen added a commit to fitzgen/dodrio that referenced this pull request Jan 16, 2019
This is a temporary stop gap until we have local js snippets:
rustwasm/rfcs#6
@xtuc
Copy link
Member

xtuc commented Jan 16, 2019

Sorry I don't have the bandwith to catch up on the comments.

I just have a note about importing a Table/Memory, if we emit the imports directly (table or memory) Webpack won't be able to bundle the module. Is the intention to handle it in the JS glue or has a Wasm import? (That something we should fix in Webpack anyway).

@fitzgen
Copy link
Member

fitzgen commented Jan 29, 2019

The inline_js would mean that the value is a string literal which the extern block imports from, and then wasm-bindgen-the-CLI would take care of placing everything in the right place on the filesystem and hooking up imports.

This sounds great to me. @koute, would this work for stdweb's js! macro?

@alexcrichton
Copy link
Contributor Author

I've added some texdt for an inline_js attribute

@koute
Copy link

koute commented Jan 29, 2019

@koute Did you see my earlier message?

With regard to stdweb, I had an idea: what if stdweb took all of the js! snippets, concatenated them together, then put them into the wasm-bindgen custom section? It could do the same for the stdweb runtime.
That way stdweb doesn't need to create hundreds of files (in fact it doesn't need to create any files at all). What do you think @koute?

I can do stuff like that in cargo-web, but I don't think stdweb itself would be able to do that by itself as every procedural macro invocation is independent of each other, and there is no post-build pass which could concatenate all of the snippets. The most I could do is to emit the snippets in a custom section which the compiler will concat in some random order (and I have such implementation in my git history already), but the point of this RFC is to get wasm-bindgen to handle the low level details, right? (:

Running a macro locally it looks like you're right and I forgot that OUT_DIR isn't set for packages which don't have a build script. Bummer!

Yep. If I may offer a suggestion - it would actually be great if cargo would always set $CARGO_TARGET_DIR. That seems pretty natural to me as setting the $CARGO_TARGET_DIR is already well established as a method of overriding the location of the target directory, and it seems like a slightly better fit than $OUT_DIR when it comes to generating files meant for consumption by external tools. I mean - everyone knows where to find the target directory, but it's not obvious how an external tool is supposed to know where $OUT_DIR is supposed to be. (Do we even have any guarantees regarding what path the $OUT_DIR points to?)

The inline_js would mean that the value is a string literal which the extern block imports from, and then wasm-bindgen-the-CLI would take care of placing everything in the right place on the filesystem and hooking up imports.

This sounds great to me. @koute, would this work for stdweb's js! macro?

Hmm... I think that would probably work, yes!

@fitzgen
Copy link
Member

fitzgen commented Jan 30, 2019

Great! Then I am in favor of moving forward with this RFC; will check my box :)

@alexcrichton
Copy link
Contributor Author

ping @ashleygwilliams, wanted to follow up on if you had thoughts on the FCP proposal above

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Feb 26, 2019
This commit is an implementation of [RFC 6] which enables crates to
inline local JS snippets into the final output artifact of
`wasm-bindgen`. This is accompanied with a few minor breaking changes
which are intended to be relatively minor in practice:

* The `module` attribute disallows paths starting with `./` and `../`.
  It requires paths starting with `/` to actually exist on the filesystem.
* The `--browser` flag no longer emits bundler-compatible code, but
  rather emits an ES module that can be natively loaded into a browser.

Otherwise be sure to check out [the RFC][RFC 6] for more details, and
otherwise this should implement at least the MVP version of the RFC!
Notably at this time JS snippets with `--nodejs` or `--no-modules` are
not supported and will unconditionally generate an error.

[RFC 6]: rustwasm/rfcs#6
@alexcrichton
Copy link
Contributor Author

I have an initial implementation of this RFC available at rustwasm/wasm-bindgen#1295

@ashleygwilliams it's been another two weeks, so I'll try pinging again

@fitzgen
Copy link
Member

fitzgen commented Feb 26, 2019

🔔 🔔 🔔

This RFC has entered its final comment period. In seven calendar days, assuming no substantial new arguments or ideas are raised, we will merge it.

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Feb 26, 2019
This commit is an implementation of [RFC 6] which enables crates to
inline local JS snippets into the final output artifact of
`wasm-bindgen`. This is accompanied with a few minor breaking changes
which are intended to be relatively minor in practice:

* The `module` attribute disallows paths starting with `./` and `../`.
  It requires paths starting with `/` to actually exist on the filesystem.
* The `--browser` flag no longer emits bundler-compatible code, but
  rather emits an ES module that can be natively loaded into a browser.

Otherwise be sure to check out [the RFC][RFC 6] for more details, and
otherwise this should implement at least the MVP version of the RFC!
Notably at this time JS snippets with `--nodejs` or `--no-modules` are
not supported and will unconditionally generate an error.

[RFC 6]: rustwasm/rfcs#6
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Feb 28, 2019
This commit is an implementation of [RFC 6] which enables crates to
inline local JS snippets into the final output artifact of
`wasm-bindgen`. This is accompanied with a few minor breaking changes
which are intended to be relatively minor in practice:

* The `module` attribute disallows paths starting with `./` and `../`.
  It requires paths starting with `/` to actually exist on the filesystem.
* The `--browser` flag no longer emits bundler-compatible code, but
  rather emits an ES module that can be natively loaded into a browser.

Otherwise be sure to check out [the RFC][RFC 6] for more details, and
otherwise this should implement at least the MVP version of the RFC!
Notably at this time JS snippets with `--nodejs` or `--no-modules` are
not supported and will unconditionally generate an error.

[RFC 6]: rustwasm/rfcs#6
@alexcrichton
Copy link
Contributor Author

The week has now lapsed with no further comments, so I will merge!

@alexcrichton alexcrichton merged commit 1194a8e into rustwasm:master Mar 5, 2019
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Mar 5, 2019
This commit is an implementation of [RFC 6] which enables crates to
inline local JS snippets into the final output artifact of
`wasm-bindgen`. This is accompanied with a few minor breaking changes
which are intended to be relatively minor in practice:

* The `module` attribute disallows paths starting with `./` and `../`.
  It requires paths starting with `/` to actually exist on the filesystem.
* The `--browser` flag no longer emits bundler-compatible code, but
  rather emits an ES module that can be natively loaded into a browser.

Otherwise be sure to check out [the RFC][RFC 6] for more details, and
otherwise this should implement at least the MVP version of the RFC!
Notably at this time JS snippets with `--nodejs` or `--no-modules` are
not supported and will unconditionally generate an error.

[RFC 6]: rustwasm/rfcs#6

Closes rustwasm#1311
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Mar 7, 2019
This commit reverts part of the implementation of [RFC 6]. That RFC
specified that the `--browser` flag was going to be repurposed for the
new "natively loadable as ES module output", but unfortunately the
breakage is far broader than initially expected. It turns out that
`wasm-pack` passes `--browser` by default which means that a change to
break `--browser` would break all historical versions of `wasm-pack`
which is a bit much for now.

To solve this the `--browser` flag is going back to what it represents
on the current released version of `wasm-bindgen` (optimize away some
node.js checks in a few places for bundler-style output) and a new
`--web` flag is being introduced as the new deployment strategy.

[RFC 6]: rustwasm/rfcs#6

Closes rustwasm#1318
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Mar 7, 2019
This commit reverts part of the implementation of [RFC 6]. That RFC
specified that the `--browser` flag was going to be repurposed for the
new "natively loadable as ES module output", but unfortunately the
breakage is far broader than initially expected. It turns out that
`wasm-pack` passes `--browser` by default which means that a change to
break `--browser` would break all historical versions of `wasm-pack`
which is a bit much for now.

To solve this the `--browser` flag is going back to what it represents
on the current released version of `wasm-bindgen` (optimize away some
node.js checks in a few places for bundler-style output) and a new
`--web` flag is being introduced as the new deployment strategy.

[RFC 6]: rustwasm/rfcs#6

Closes rustwasm#1318
@alexcrichton
Copy link
Contributor Author

As a heads up after landing the initial implementation it's been discovered that wasm-pack is by default passing --browser (this was unknown before) for all compilations. This means that the proposed semantics in this exact RFC of breaking --browser aren't gonna fly, so I've proposed a change where the new semantics of the --browser flag are instead introduced as --web instead of repurposing the --browser flag

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Mar 7, 2019
This commit reverts part of the implementation of [RFC 6]. That RFC
specified that the `--browser` flag was going to be repurposed for the
new "natively loadable as ES module output", but unfortunately the
breakage is far broader than initially expected. It turns out that
`wasm-pack` passes `--browser` by default which means that a change to
break `--browser` would break all historical versions of `wasm-pack`
which is a bit much for now.

To solve this the `--browser` flag is going back to what it represents
on the current released version of `wasm-bindgen` (optimize away some
node.js checks in a few places for bundler-style output) and a new
`--web` flag is being introduced as the new deployment strategy.

[RFC 6]: rustwasm/rfcs#6

Closes rustwasm#1318
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Mar 15, 2019
This allows subverting the checks and resolution performed by the
`module` attribute added as part of [RFC 6] and has been discussed in rustwasm#1343.

Closes rustwasm#1343

[RFC 6]: rustwasm/rfcs#6
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Mar 15, 2019
This allows subverting the checks and resolution performed by the
`module` attribute added as part of [RFC 6] and has been discussed in rustwasm#1343.

Closes rustwasm#1343

[RFC 6]: rustwasm/rfcs#6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants