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

Various quality of life improvements. #63

Merged
merged 3 commits into from
Jul 2, 2019
Merged

Conversation

Pauan
Copy link
Contributor

@Pauan Pauan commented Jun 15, 2019

Here is a list of changes in this PR:

  • Adding in outDir and outName options.

  • It now watches Cargo.toml for changes.

  • It fixes Webpack erroring about pkg/index.js if wasm-pack errors.

  • Changing to use Webpack's error system, which causes Webpack compilation to gracefully stop if there is an error.

The end result is a much nicer experience for the end user:

  • It no longer spews out a lot of useless Webpack information when compilation fails.

  • It works perfectly in watch mode, with full auto-reloading, even in the case that there are errors in the Rust code.

  • The user can now edit Cargo.toml and it will auto-rebuild.

However, this is a BREAKING CHANGE, because it now defaults --out-dir to pkg, and defaults --out-name to index. This is necessary in order to fix a Webpack error. Therefore, this will need a major version bump.

@Pauan
Copy link
Contributor Author

Pauan commented Jun 25, 2019

@xtuc Ping.

@xtuc
Copy link
Member

xtuc commented Jun 26, 2019

I have this on my radar, sorry for the delay.

.gitignore Show resolved Hide resolved
@xtuc
Copy link
Member

xtuc commented Jun 26, 2019

Thanks for your PR!

However, this is a BREAKING CHANGE, because it now defaults --out-dir to pkg, and defaults --out-name to index. This is necessary in order to fix a Webpack error. Therefore, this will need a major version bump.

I'm not sure to understand what is breaking? You mean people that passed -- --out-dir ... for example? Apart from that it looks like it's compatible.

@Pauan
Copy link
Contributor Author

Pauan commented Jun 26, 2019

I'm not sure to understand what is breaking? You mean people that passed -- --out-dir ... for example?

Yes, that does cause breakage. But the other breakage is that it now defaults to pkg/index.js for the filename. Before, it would default to pkg/{name}.js, where {name} is the name of the package.

So for example, if somebody has a package called foo, then their Webpack will be configured to load ./pkg/foo.js, and now that will break, since it's now outputting to ./pkg/index.js

That means basically all existing users of wasm-pack-plugin will have their build broken. Hence why it needs a major version bump, to avoid that.

@Pauan
Copy link
Contributor Author

Pauan commented Jun 27, 2019

By the way, I've been thinking about how to support Rust crates which have multiple binaries. So there will probably be some more breaking changes soon, so we should probably hold off on merging this PR until that's done.

@xtuc
Copy link
Member

xtuc commented Jun 28, 2019

So for example, if somebody has a package called foo, then their Webpack will be configured to load ./pkg/foo.js, and now that will break, since it's now outputting to ./pkg/index.js

wasm-pack emits a package.json file with the main field (or module but it's the same in our case) being the name of the file, otherwise it wouldn't work at all. I assume that, by passing the new arguments, wasm-pack will indicate index.js instead. I'm almost sure it won't cause any breakage, you can test using the example (make sure to change the wasm-pack-plugin dependecies to ../ in the webpack config to test).

@xtuc
Copy link
Member

xtuc commented Jun 28, 2019

It seems that I have an outdate version of wasm-bindgen:

Unknown flag: '--out-name'

Usage:
    wasm-bindgen [options] <input>
    wasm-bindgen -h | --help
    wasm-bindgen -V | --version
Error: Running the wasm-bindgen CLI
Caused by: failed to execute `wasm-bindgen`: exited with exit code: 1

Would it be possible, if you know, to document the versin needed? Thanks!

I'm not able to make the example work again, but your PR give better UX!

@Pauan
Copy link
Contributor Author

Pauan commented Jun 28, 2019

wasm-pack emits a package.json file with the main field (or module but it's the same in our case) being the name of the file, otherwise it wouldn't work at all. I assume that, by passing the new arguments, wasm-pack will indicate index.js instead.

There is no requirement for the user to use the package.json file. They can (and do) import the JS files directly.

For example, in the hello_world demo in wasm-bindgen:

  1. In webpack.config.js it imports ./index.js

  2. In index.js it imports ./pkg/hello_world

This works because the package's name is hello_world, and so wasm-pack generates a pkg/hello_world.js file.

But that will now break with this PR, since it now generates a pkg/index.js file instead, so the import fails. The package.json file is completely bypassed, so it doesn't matter what the "main" field is.

Would it be possible, if you know, to document the versin needed? Thanks!

It seems to be 0.8.0, released on April 2nd. I'll update the PR.

@Pauan
Copy link
Contributor Author

Pauan commented Jun 28, 2019

So I did some experiments with making wasm-pack-plugin work with multiple crates, and it didn't work out very good.

Both Webpack and wasm-pack aren't really designed for that use-case, and so I think Webpack plugins are a bad fit.

Instead, I created a Webpack loader in ~200 lines of code which works fantastic:

  • It has excellent support for multiple crates

  • It is even more ergonomic than this PR

  • It avoids all the issues with --out-dir and --out-name

  • It has less boilerplate for the user

  • It uses Webpack's watcher rather than a custom watcher

  • It handles errors even better

  • It doesn't use wasm-pack at all, which means less issues for the user (they don't need to install wasm-pack or keep it up to date)

Basically, it's better in essentially every way, with no downsides (currently). I'm interested in uploading the code to @wasm-tool/rust-loader, but that repo was taken down.

I still think this PR has merit, since it helps out the current users of wasm-pack-plugin, but since I need multiple-crate support I'm going to be focusing on the Webpack loader instead.

@Pauan
Copy link
Contributor Author

Pauan commented Jun 28, 2019

(I added in a note about the minimum wasm-pack version)

@xtuc
Copy link
Member

xtuc commented Jun 29, 2019

Basically, it's better in essentially every way, with no downsides (currently). I'm interested in uploading the code to @wasm-tool/rust-loader, but that repo was taken down.

Really cool! I recreated the repo and added you as collab. At first I initially created a loader but had issues with the hotreloading If I remember correctly.

I'm happy to switch back to a loader if it adds improvements and better UX, thanks for your work!

@xtuc
Copy link
Member

xtuc commented Jun 29, 2019

I'm not able to run the example because it's upgrading some deps and I'm running into rustwasm/wasm-bindgen#1613. I'm generally ok with your PR, I just want to upgrade the example and test it.

@Pauan
Copy link
Contributor Author

Pauan commented Jun 29, 2019

I recreated the repo and added you as collab. At first I initially created a loader but had issues with the hotreloading If I remember correctly.

Great, thanks! I'll make sure to test hotreloading to see if it works correctly.


You can fix that wasm-bindgen error by setting this in your Cargo.toml:

[dependencies]
wasm-bindgen = "=0.2.45"

Then wasm-pack should auto-download the right version of wasm-bindgen.

@Pauan
Copy link
Contributor Author

Pauan commented Jun 30, 2019

So I tested hot reloading with @wasm-tool/rust-loader and I ran into what appears to be Webpack bugs: it keeps thinking a file doesn't exist even though it does, or using stale files, etc. I don't think there's anything we can do about that.

@xtuc
Copy link
Member

xtuc commented Jul 2, 2019

Thanks, I will try to take a look at it.

I went ahead and updated the demo on your branch.

}

apply(compiler) {
this.isDebug = this.forceMode ? this.forceMode === "development" : compiler.options.mode === "development";

// This fixes an error in Webpack where it cannot find
// the `pkg/index.js` file if Rust compilation errors.
this._makeEmpty();
Copy link
Member

@xtuc xtuc Jul 2, 2019

Choose a reason for hiding this comment

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

If you already imported an non-existing module with webpack, it will tell you in the console and at runtime, It has quite a good UX IMO. We could create a file with a ThrowStatement with the compilation error in it.

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 don't think the UX is good. This is what you get with the old version:

error[E0599]: no function or associated item named `from_str2` found for type `wasm_bindgen::JsValue` in the current scope
  --> src\lib.rs:24:30
   |
24 |     console::log_1(&JsValue::from_str2("Hello world!"));
   |                              ^^^^^^^^^
   |                              |
   |                              function or associated item not found in `wasm_bindgen::JsValue`
   |                              help: there is a method with a similar name: `from_str`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0599`.
error: Could not compile `rust-webpack-template`.

To learn more, run the command again with --verbose.
Error: Compiling your crate to WebAssembly failed
Caused by: failed to execute `cargo build`: exited with exit code: 101
wasm-pack error: undefined
Hash: 3b93c8acec8101b6a7f9
Version: webpack 4.35.2
Time: 1818ms
Built at: 07/02/2019 2:46:07 PM
 1 asset
Entrypoint index = index.js
[0] ./js/index.js 49 bytes {0} [built]

ERROR in ./js/index.js
Module not found: Error: Can't resolve '../pkg/index.js' in 'C:\Users\Pauan\Shared Folders\NixOS\tmp\my-app\js'
 @ ./js/index.js 1:0-25
error Command failed with exit code 2.

This part here is useless, and it actually pushes away the useful information:

ERROR in ./js/index.js
Module not found: Error: Can't resolve '../pkg/index.js' in 'C:\Users\Pauan\Shared Folders\NixOS\tmp\my-app\js'
 @ ./js/index.js 1:0-25

So my changes gets rid of it. You still get an error message, just without the useless stuff.

Copy link
Member

@xtuc xtuc Jul 2, 2019

Choose a reason for hiding this comment

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

Sorry, I typed the comment too fast. I just wanted to say that we could show the error on the browser-side as well, by injecting code with a ThrowStatement in ./pkg/index.js.

Also, the relevant source is a couple of line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for putting the compilation error into a file (or whatever), I tried that but it's not possible.

When you spawn a command (like wasm-pack build) you have two options: you can pipe the output, or you can inherit the stdin/stdout/stderr.

If you pipe the output, then you can store it in a file, but then you don't get any colors from cargo build.

If you inherit the output, then you get nice colors with cargo build, but then you can't retrieve the output, so you can't put it into a file.

I tried all sorts of workarounds, nothing worked. So I gave up.

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

Thank you for your work!

@xtuc xtuc merged commit acf57a7 into wasm-tool:master Jul 2, 2019
@xtuc
Copy link
Member

xtuc commented Jul 2, 2019

@wasm-tool/wasm-pack-plugin@1.0.0 has been published!

@Pauan
Copy link
Contributor Author

Pauan commented Jul 2, 2019

@xtuc The error was Webpack saying it can't resolve the file (even though the file clearly existed).

And whenever the file changed, Webpack wouldn't pick up the changes, it would keep using the old stale file.

This only happened with hot module reloading. Otherwise it worked perfectly.

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.

2 participants