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

Implement the local JS snippets RFC #1295

Merged
merged 18 commits into from
Mar 5, 2019

Conversation

alexcrichton
Copy link
Contributor

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

@koute
Copy link

koute commented Feb 26, 2019

Awesome! I'll try to port stdweb to this in a few days and we'll see how it goes.

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
The cwd is different for workspaces, so use the manifest env var
instead.
When importing a file across multiple locations in a module make sure it
doesn't trip an assert and it works as expected.
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.

Thanks @alexcrichton :)

crates/backend/src/codegen.rs Show resolved Hide resolved
crates/backend/src/encode.rs Outdated Show resolved Hide resolved
if contents.starts_with("class") {
format!("{1}\n__exports.{0} = {0};\n", name, contents)
} else {
format!("__exports.{} = {};\n", name, contents)
Copy link
Member

Choose a reason for hiding this comment

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

We don't actually need to have two different cases here, since this is valid JS:

__exports.MyClass = class MyClass { ... };

Same for the other bundling modes.

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 was unwaware! When I started to do this though it looks like that idiom doesn't also define a MyClass identifier in the current scope which the codegen currently relies on (but export class Foo {... } does declare Foo in the local scope which is what we need)

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha.

crates/cli-support/src/js/mod.rs Outdated Show resolved Hide resolved
crates/cli-support/src/lib.rs Outdated Show resolved Hide resolved
examples/without-a-bundler-no-modules/README.md Outdated Show resolved Hide resolved
examples/without-a-bundler-no-modules/README.md Outdated Show resolved Hide resolved
examples/without-a-bundler/build.sh Show resolved Hide resolved
examples/without-a-bundler/index.html Outdated Show resolved Hide resolved
fitzgen and others added 8 commits March 5, 2019 12:30
Co-Authored-By: alexcrichton <alex@alexcrichton.com>
Co-Authored-By: alexcrichton <alex@alexcrichton.com>
It's already hidden from docs!
Using `unsafe` was just a little too eager there so let's use an
off-the-shelf solution for solving the actual problem we have, which is
to allocate strings with a lifetime of `Interner` rather than
deduplicating strings.
Use the same crate identifier for manually included snippets as well as
inline snippets to help with debugging.
@alexcrichton
Copy link
Contributor Author

Thanks for the thorough review @fitzgen! (and so quick!)

@alexcrichton alexcrichton merged commit 20c25ca into rustwasm:master Mar 5, 2019
@alexcrichton alexcrichton deleted the js-snippets branch March 5, 2019 23:18
This was referenced Mar 5, 2019
@Vlad-Shcherbina
Copy link
Contributor

This looks like a substantial change. Maybe it should be mentioned in the 'Unreleased' section of the changelog.

@alexcrichton
Copy link
Contributor Author

@Vlad-Shcherbina indeed yeah! We typically don't do a great job of keeping the changelog up to date as things land, but this'll definitely be mentioned for the next release!

@Vlad-Shcherbina
Copy link
Contributor

Whatever process works for you, obviously.
But in general it's a good practice to update 'Unreleased' section together with the code.
https://keepachangelog.com/en/1.0.0/#effort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants