Skip to content

Commit

Permalink
hack around node windows unicode bug (#687)
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Jan 26, 2021
1 parent 88c8523 commit 93423e5
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 8 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@

The workaround was to manually check for this case and then ignore the error in this specific case. With this release, it should now be possible to pipe something to the `esbuild` command on Windows.

* Fix stdout and stderr not supporting Unicode in the `esbuild-wasm` package on Windows ([#687](https://github.com/evanw/esbuild/issues/687))

Node's `fs.write` API is broken when writing Unicode to stdout and stderr on Windows, and this will never be fixed: [nodejs/node#24550](https://github.com/nodejs/node/issues/24550). This is problematic for Go's WebAssembly implementation because it uses this API for writing to all file descriptors.

The workaround is to manually intercept the file descriptors for stdout and stderr and redirect them to `process.stdout` and `process.stderr` respectively. Passing Unicode text to `write()` on these objects instead of on the `fs` API strangely works fine. So with this release, Unicode text should now display correctly when using esbuild's WebAssembly implementation on Windows (or at least, as correctly as the poor Unicode support in Windows Command Prompt allows).
* Add a hack for faster command-line execution for the WebAssembly module in certain cases
Node has an unfortunate bug where the node process is unnecessarily kept open while a WebAssembly module is being optimized: https://github.com/nodejs/node/issues/36616. This means cases where running `esbuild` should take a few milliseconds can end up taking many seconds instead.
Expand Down
40 changes: 32 additions & 8 deletions scripts/esbuild.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,37 @@ exports.buildWasmLib = async (esbuildPath) => {
fs.mkdirSync(esmDir, { recursive: true })

// Generate "npm/esbuild-wasm/wasm_exec.js"
const toReplace = 'global.fs = fs;';
const GOROOT = childProcess.execFileSync('go', ['env', 'GOROOT']).toString().trim();
fs.copyFileSync(
path.join(GOROOT, 'misc', 'wasm', 'wasm_exec.js'),
path.join(npmWasmDir, 'wasm_exec.js'),
);
let wasm_exec_js = fs.readFileSync(path.join(GOROOT, 'misc', 'wasm', 'wasm_exec.js'), 'utf8');
let index = wasm_exec_js.indexOf(toReplace);
if (index === -1) throw new Error(`Failed to find ${JSON.stringify(toReplace)} in Go JS shim code`);
wasm_exec_js = wasm_exec_js.replace(toReplace, `
global.fs = Object.assign({}, fs, {
// Hack around a bug in node: https://github.com/nodejs/node/issues/24550
writeSync(fd, buf) {
if (fd === process.stdout.fd) return process.stdout.write(buf), buf.length;
if (fd === process.stderr.fd) return process.stderr.write(buf), buf.length;
return fs.writeSync(fd, buf);
},
write(fd, buf, offset, length, position, callback) {
if (offset === 0 && length === buf.length && position === null) {
if (fd === process.stdout.fd) {
try { process.stdout.write(buf); }
catch (err) { return callback(err, 0, null); }
return callback(null, length, buf);
}
if (fd === process.stderr.fd) {
try { process.stderr.write(buf); }
catch (err) { return callback(err, 0, null); }
return callback(null, length, buf);
}
}
fs.write(fd, buf, offset, length, position, callback);
},
});
`);
fs.writeFileSync(path.join(npmWasmDir, 'wasm_exec.js'), wasm_exec_js);

// Generate "npm/esbuild-wasm/lib/main.js"
childProcess.execFileSync(esbuildPath, [
Expand All @@ -80,13 +106,11 @@ exports.buildWasmLib = async (esbuildPath) => {
const minifyFlags = minify ? ['--minify'] : []

// Process "npm/esbuild-wasm/wasm_exec.js"
const wasm_exec_js = path.join(npmWasmDir, 'wasm_exec.js')
let wasmExecCode = fs.readFileSync(wasm_exec_js, 'utf8');
let wasmExecCode = wasm_exec_js;
if (minify) {
const wasmExecMin = childProcess.execFileSync(esbuildPath, [
wasm_exec_js,
'--target=es2015',
].concat(minifyFlags), { cwd: repoDir }).toString()
].concat(minifyFlags), { cwd: repoDir, input: wasmExecCode }).toString()
const commentLines = wasmExecCode.split('\n')
const firstNonComment = commentLines.findIndex(line => !line.startsWith('//'))
wasmExecCode = '\n' + commentLines.slice(0, firstNonComment).concat(wasmExecMin).join('\n')
Expand Down
34 changes: 34 additions & 0 deletions scripts/wasm-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,40 @@ const tests = {
assert.deepStrictEqual(exports.default, 3);
},

stdinStdoutUnicodeTest({ testDir, esbuildPath }) {
const stdout = child_process.execFileSync('node', [
esbuildPath,
'--format=cjs',
], {
stdio: ['pipe', 'pipe', 'inherit'],
cwd: testDir,
input: `export default ['π', '🍕']`,
}).toString();

// Check that the bundle is valid
const module = { exports: {} };
new Function('module', 'exports', stdout)(module, module.exports);
assert.deepStrictEqual(module.exports.default, ['π', '🍕']);
},

stdinOutfileUnicodeTest({ testDir, esbuildPath }) {
const outfile = path.join(testDir, 'out.js')
child_process.execFileSync('node', [
esbuildPath,
'--bundle',
'--format=cjs',
'--outfile=' + outfile,
], {
stdio: ['pipe', 'pipe', 'inherit'],
cwd: testDir,
input: `export default ['π', '🍕']`,
}).toString();

// Check that the bundle is valid
const exports = require(outfile);
assert.deepStrictEqual(exports.default, ['π', '🍕']);
},

importRelativeFileTest({ testDir, esbuildPath }) {
const outfile = path.join(testDir, 'out.js')
const packageJSON = path.join(__dirname, '..', 'npm', 'esbuild-wasm', 'package.json');
Expand Down

7 comments on commit 93423e5

@kzc
Copy link
Contributor

@kzc kzc commented on 93423e5 Jan 26, 2021

Choose a reason for hiding this comment

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

Is global.fs monkey patched on all platforms - not just Windows?

@evanw
Copy link
Owner Author

@evanw evanw commented on 93423e5 Jan 26, 2021

Choose a reason for hiding this comment

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

Yes. I decided to do that because they should be equivalent and doing this on all platforms increases the test coverage of this patch. If there's something that isn't equivalent about this change, it'd be good to know sooner rather than later.

Note that the global variable is only the global object when running via esbuild-wasm/bin/esbuild, which is always run in a child process via execFile() since it calls process.exit(). So this variable won't be accessible from the host process when using esbuild's JavaScript API from node. And when running in the browser, the global variable is a clone of the global object instead for safety, so it's not accessible when using esbuild's JavaScript API in the browser either:

esbuild/lib/browser.ts

Lines 39 to 46 in b8f1e7b

let code = `{` +
`let global={};` +
`for(let o=self;o;o=Object.getPrototypeOf(o))` +
`for(let k of Object.getOwnPropertyNames(o))` +
`if(!(k in global))` +
`Object.defineProperty(global,k,{get:()=>self[k]});` +
WEB_WORKER_SOURCE_CODE +
`}`

@kzc
Copy link
Contributor

@kzc kzc commented on 93423e5 Jan 26, 2021

Choose a reason for hiding this comment

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

Yes. I decided to do that because they should be equivalent and doing this on all platforms increases the test coverage of this patch. If there's something that isn't equivalent about this change, it'd be good to know sooner rather than later.

Good call... 64KB buffer boundary heisenbug found on Mac built from b8f1e7b:

$ node -v
v14.3.0

$ esbuild scripts/js-api-tests.js | wc -c
   98228

$ npm/esbuild-wasm/bin/esbuild scripts/js-api-tests.js | wc -c
   73728
$ npm/esbuild-wasm/bin/esbuild scripts/js-api-tests.js | wc -c
   65536
$ npm/esbuild-wasm/bin/esbuild scripts/js-api-tests.js | wc -c
   73728
$ npm/esbuild-wasm/bin/esbuild scripts/js-api-tests.js | wc -c
   65536

Curiously, when stdout redirected to file, it's okay:

$ esbuild scripts/js-api-tests.js > 0.js

$ npm/esbuild-wasm/bin/esbuild scripts/js-api-tests.js > 1.js

$ shasum 0.js 1.js
81ddd135a8c29734c915d6366d9559fa41ba36a0  0.js
81ddd135a8c29734c915d6366d9559fa41ba36a0  1.js

$ wc -c 0.js 1.js
   98228 0.js
   98228 1.js

@kzc
Copy link
Contributor

@kzc kzc commented on 93423e5 Jan 26, 2021

Choose a reason for hiding this comment

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

The esbuild-wasm problem does not appear with binaries built from 88c8523.

@evanw
Copy link
Owner Author

@evanw evanw commented on 93423e5 Jan 26, 2021

Choose a reason for hiding this comment

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

Thanks for catching this. I'm guessing process.stdout.write() isn't synchronous like I thought it was. Go reaches the end of main and calls exit(0) before process.stdout.write() finishes, thus causing us to exit the process while node is still writing to stdout. I think adding a callback to process.stdout.write() should fix this.

@evanw
Copy link
Owner Author

@evanw evanw commented on 93423e5 Jan 26, 2021

Choose a reason for hiding this comment

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

I think this should be addressed by 392241b.

@evanw
Copy link
Owner Author

@evanw evanw commented on 93423e5 Jan 26, 2021

Choose a reason for hiding this comment

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

The fix is published as 0.8.36. Thank you very much for the bug report.

Please sign in to comment.