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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
node_modules/
package-lock.json
yarn.lock
xtuc marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ module.exports = {
// path.resolve(__dirname, "another-crate/src")
// ],

// The same as the `--out-dir` option for `wasm-pack`
// outDir: "pkg",

// The same as the `--out-name` option for `wasm-pack`
// outName: "index",

// If defined, `forceWatch` will force activate/deactivate watch mode for
// `.rs` files.
//
Expand Down
102 changes: 79 additions & 23 deletions plugin.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const {
spawn
} = require('child_process');
const fs = require('fs');
const path = require('path');
const commandExistsSync = require('command-exists').sync;
const chalk = require('chalk');
Expand All @@ -23,17 +24,25 @@ class WasmPackPlugin {
this.crateDirectory = options.crateDirectory;
this.forceWatch = options.forceWatch;
this.forceMode = options.forceMode;
this.extraArgs = (options.extraArgs || '').trim().split(' ').filter(x=> x);
this.extraArgs = (options.extraArgs || '').trim().split(' ').filter(x => x);
this.outDir = options.outDir || "pkg";
this.outName = options.outName || "index";
this.watchDirectories = (options.watchDirectories || [])
.concat(path.resolve(this.crateDirectory, 'src'));
this.watchFiles = [path.resolve(this.crateDirectory, 'Cargo.toml')];

this.wp = new Watchpack();
this.isDebug = true;
this.error = null;
}

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.


// force first compilation
compiler.hooks.beforeCompile.tapPromise('WasmPackPlugin', () => {
if (this._ranInitialCompilation === true) {
Expand All @@ -44,16 +53,50 @@ class WasmPackPlugin {

return this._checkWasmPack()
.then(() => {
if (this.forceWatch || (this.forceWatch === undefined && compiler.watchMode)) {
this.wp.watch([], this.watchDirectories, Date.now() - 10000);
this.wp.on('change', this._compile.bind(this));
}
return this._compile();
})
.catch(this._compilationFailure);
const shouldWatch = this.forceWatch || (this.forceWatch === undefined && compiler.watchMode);

if (shouldWatch) {
this.wp.watch(this.watchFiles, this.watchDirectories, Date.now() - 10000);

this.wp.on('change', () => {
this._compile(true);
});
}

return this._compile(false);
});
});

let first = true;

compiler.hooks.thisCompilation.tap('WasmPackPlugin', (compilation) => {
// Super hacky, needed to workaround a bug in Webpack which causes
// thisCompilation to be triggered twice on the first compilation.
if (first) {
first = false;

} else {
// This is needed in order to gracefully handle errors in Webpack,
// since Webpack has its own custom error system.
if (this.error != null) {
compilation.errors.push(this.error);
}
}
});
}

_makeEmpty() {
try {
fs.mkdirSync(this.outDir);
} catch (e) {
if (e.code !== "EEXIST") {
throw e;
}
}

fs.writeFileSync(path.join(this.outDir, this.outName + ".js"), "");
}

_checkWasmPack() {
info('🧐 Checking for wasm-pack...\n');

Expand All @@ -71,32 +114,42 @@ class WasmPackPlugin {
}
}

_compile() {
_compile(watching) {
info(`ℹ️ Compiling your crate in ${this.isDebug ? 'development' : 'release'} mode...\n`);

return spawnWasmPack({
outDir: this.outDir,
outName: this.outName,
isDebug: this.isDebug,
cwd: this.crateDirectory,
extraArgs: this.extraArgs,
})
.then(this._compilationSuccess)
.catch(this._compilationFailure);
}
.then((detail) => {
// This clears out the error when the compilation succeeds.
this.error = null;

_compilationSuccess(detail) {
if (detail) {
info(detail);
}

info('✅ Your crate has been correctly compiled\n');
}
if (detail) {
info(detail);
}

_compilationFailure(err) {
error('wasm-pack error: ' + err);
info('✅ Your crate has been correctly compiled\n');
})
.catch((e) => {
// Webpack has a custom error system, so we cannot return an
// error directly, instead we need to trigger it later.
this.error = e;

if (watching) {
// This is to trigger a recompilation so it displays the error message
this._makeEmpty();
}
});
}
}

function spawnWasmPack({
outDir,
outName,
isDebug,
cwd,
extraArgs
Expand All @@ -106,8 +159,10 @@ function spawnWasmPack({
const args = [
'--verbose',
'build',
'--out-dir', outDir,
'--out-name', outName,
...(isDebug ? ['--dev'] : []),
...extraArgs,
...extraArgs
];

const options = {
Expand All @@ -125,8 +180,9 @@ function runProcess(bin, args, options) {
p.on('close', code => {
if (code === 0) {
resolve();

} else {
reject();
reject(new Error("Rust compilation."));
}
});

Expand Down
Loading