Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Add support for local JS snippets in wasm-bindgen #6
Changes from 6 commits
1c57ae8
9fe2a6f
51c09bf
1c3e9ff
b83aabb
6e6eb0a
e6d588a
4d990bb
32952b5
02c4e1e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 inpackage.json
orCargo.toml
).But with this approach, everything "Just Works", without needing to specify any metadata.
There was a problem hiding this comment.
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:
--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./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!#[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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is true, that negates one of the advantages. Though I think the other advantages are still really good!
Oh, I hadn't considered that. That's a good point. If users manually specified the
.js
files inpackage.json
, would those changes be picked up? Or would it only be changes inCargo.toml
that are picked up?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.
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 inpackage.json
orCargo.toml
.There was a problem hiding this comment.
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 applicationThere was a problem hiding this comment.
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"?
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 inpackage.json
") strategy is necessary.That sounds like it could be a useful feature even outside of wasm-bindgen!
There was a problem hiding this comment.
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:
In Rust code we'd have:
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)
There was a problem hiding this comment.
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 andimport()
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.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?
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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:
#[wasm_bindgen(module = "path")]
import { ... }
will, like#[wasm_bindgen]
, allow all three pathsimport(...)
will disallow absolute paths.import { ... }
this won't work to start off with at all and will fail with weird errorsimport { ... }
relative paths will work by shipping multiple filesSorry 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 inimport(..)
, 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are in agreement here.
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 theimport()
expression isn't good. The spec authors put a lot of effort into makingimport()
equivalent toimport
, and for good reason.And I think that we shouldn't break the already existing conventions for
.js
files (which are exclusively relative paths).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.There was a problem hiding this comment.
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!)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 expectednode
andbrowser
replace the flags suggested in this PRThere is some precedent in TypeScript too.
There was a problem hiding this comment.
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"
inimport x from "foo"
). For us though it's more about what to generate at all, e.g.import
orrequire
or "somehow nothing"There was a problem hiding this comment.
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 ofmodule-resolution
?browser
andnode
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.There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 newimport
, will that change be picked up?There was a problem hiding this comment.
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. Forimport
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)There was a problem hiding this comment.
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 ifwasm-bindgen
usesinclude_str!
forpackage.json
, then if somebody editspackage.json
to add a new.js
file, the change will be picked up?There was a problem hiding this comment.
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
inwasm-bindgen
, onlywasm-pack
. Inwasm-pack
everything is unconditionally done all the time (as it's so fast), but if we were to need to parse things inwasm-bindgen
this'd be how we do it!There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 thewasm-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 duringwasm-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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thepackage.json
in the custom section, how willwasm-pack
be able to handlepackage.json
in transitive crates?Maybe we should move this conversation to another issue, since it seems to be getting quite off-topic.
There was a problem hiding this comment.
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 shippackage.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
There was a problem hiding this comment.
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.