Minification causes unexpected token '{' when targetting es5

[advdv]

Dear reader, what an amazing library!

I've ran into an issue though. When I'm bundling a main.js file with imports like this:

import $ from 'jquery'

console.log($)

with the following esbuild command

esbuild --bundle main.js --outfile=../dist/bundle.js --target=es5 --minify

and open the page in safari, i get:

SyntaxError: Unexpected token '{'. Expected an opening '(' before a function's parameter list.

and in Chrome, i get just:

Uncaught SyntaxError: Unexpected token '{'

The relevant portion in the minified javascript looks like this:

... ge=function y{r ...

Is this a bug or is this configuration combination not supported?

Edit: I run version 0.6.25 of esbuild

[kzc]

Appears to happen with any commonjs import. Reduced test case:

$ cat bar.js
module.exports = console.log("bar");

$ echo 'import "./bar.js";' | esbuild --bundle --minify-whitespace --target=es2015 | node
bar

$ echo 'import "./bar.js";' | esbuild --bundle --minify-whitespace --target=es5 | esbuild
<stdin>:1:289: error: Expected "(" but found "{"
...}};var __markAsModule=function target{return __defineProperty(target,"__es...
                                        ^
1 error

[kzc]

esbuild/internal/printer/printer.go

Lines 1500 to 1505 in 3f8508a

	if useFunction {
	p.printSpaceBeforeIdentifier()
	p.print("function")
	}
	p.printFnArgs(e.Args, e.HasRestArg, true)

The args are not parenthesized in this arrow function that is being converted to a function expression. I don't have a Go compiler at my disposal, but this might do the trick:

p.printFnArgs(e.Args, e.HasRestArg, !useFunction)

[evanw]

Thank you so much for the report, and for the suggested fix. This should be fixed in version 0.6.26.

[kzc]

@evanw A somewhat related question...

Any plans to have esbuild scope hoist commonjs modules similar to rollup?

$ cat bar.js
module.exports = console.log("bar");

$ echo 'import "./bar.js";' | esbuild --bundle --minify
(()=>{var a=Object.defineProperty,l=Object.prototype.hasOwnProperty,p=(o,r)=>()=>(r||(r={exports:{}},o(r.exports,r)),r.exports),t=o=>a(o,"__esModule",{value:!0}),c=(o,r)=>{if(t(o),typeof r=="object"||typeof r=="function")for(let s in r)!l.call(o,s)&&s!=="default"&&a(o,s,{get:()=>r[s],enumerable:!0});return o},g=o=>o&&o.__esModule?o:c(a({},"default",{value:o,enumerable:!0}),o);var e=p((j,b)=>{b.exports=console.log("bar")});const m=g(e());})();

$ echo 'import "./bar.js";' | rollup --silent -p commonjs,terser
console.log("bar");

A slightly more elaborate example:

$ cat foo.js
exports.C = 123;
exports.hi = function(x) { console.log("Hello", x); };

$ echo 'import m from "./foo.js"; m.hi(m.C);' | esbuild --bundle --minify
(()=>{var f=Object.defineProperty,e=Object.prototype.hasOwnProperty,h=(o,i)=>()=>(i||(i={exports:{}},o(i.exports,i)),i.exports),r=o=>f(o,"__esModule",{value:!0}),s=(o,i)=>{if(r(o),typeof i=="object"||typeof i=="function")for(let l in i)!e.call(o,l)&&l!=="default"&&f(o,l,{get:()=>i[l],enumerable:!0});return o},t=o=>o&&o.__esModule?o:s(f({},"default",{value:o,enumerable:!0}),o);var c=h(m=>{m.C=123;m.hi=function(o){console.log("Hello",o)}});const n=t(c());n.default.hi(n.default.C);})();

$ echo 'import m from "./foo.js"; m.hi(m.C);' | rollup --silent -p commonjs,terser
(function(o){console.log("Hello",o)})(123);

[evanw]

I have some ideas for how to do this transformation safely (AFAIK Rollup’s transformation isn’t safe because it doesn’t preserve order and may miss some exports) but they are not high priority for me. They add pretty significant complexity to linking and I’d rather spend that complexity budget on moving forward with code splitting and plugins. I don’t want to do exactly what Rollup does with trying to convert CommonJS to ESM because I want esbuild to “just work” with CommonJS modules without introducing correctness issues. Converting CommonJS to ESM is a lossy conversion because there is a semantic mismatch.

FWIW my current ideas are along the lines of inlining the CommonJS closure into the first call to require for that module as long as the call to require can be made to be in its own top-level statement, and the bundler can prove that no other call to require for that module can be evaluated before the one deemed first. Then the closure would be unwrapped and the exports and/or module arguments would become top-level variables. All subsequent calls to require would just reference one of those variables.

This introduces a few complexities. One is the analysis to determine whether inlining is safe. Another is that modules would no longer necessarily be contiguous in the generated code because they may be split at top-level require calls. This has implications for source maps, for example. And even with all of this complexity, it doesn’t seem like it will improve things that much, at least not with an advanced partial evaluation scheme that tries to determine if there are unreferenced object properties assigned to objects, which is a whole other level of complexity.

Ultimately I see the ecosystem moving to ESM over time, which will make this less and less of an issue without needing to do this work.

[kzc]

AFAIK Rollup’s transformation isn’t safe because it doesn’t preserve order and may miss some exports

I've never encountered that. It ought to preserve side effects of modules. If the module is side effect free then it's safe to drop unused commonjs exports. If you have a failing test case please file an issue with rollup.

[evanw]

I tried to look up how this transform works and I actually couldn't find any documentation (I checked here and here). So I could definitely be mistaken. Sorry if this information was incorrect.

From what I understand, Rollup converts require() calls to import statements and while CommonJS has a very specific behavior for the order in which modules are evaluated, ES6 only specifies that imported modules must be evaluated before the importer module but doesn't specify the specific evaluation order. So transforming CommonJS to ES6 like this loosens the requirements around import evaluation order. I could be wrong on this and it'd be great to know if so.

I've experienced cases where Rollup's CommonJS plugin misses exports by default. This is documented here in Rollup's documentation:

Occasionally you will see an error message like this:

'foo' is not exported by bar.js (imported by baz.js)

Import declarations must have corresponding export declarations in the imported module. For example, if you have import a from './a.js' in a module, and a.js doesn't have an export default declaration, or import {foo} from './b.js', and b.js doesn't export foo, Rollup cannot bundle the code.

This error frequently occurs with CommonJS modules converted by @rollup/plugin-commonjs, which makes a reasonable attempt to generate named exports from the CommonJS code but won't always succeed, because the freewheeling nature of CommonJS is at odds with the rigorous approach we benefit from in JavaScript modules. It can be solved by using the namedExports option, which allows you to manually fill in the information gaps.

We experienced this together here where Rollup's CommonJS plugin didn't work correctly with the react package. The fix was to make this change to the Rollup config:

-  commonjs(),
+  commonjs({
+    include: 'node_modules/**',
+    namedExports: {
+      react: Object.keys(react),
+      'react-dom': Object.keys(reactDom)
+    }
+  }),

I'm planning to avoid adding transforms in esbuild that require manual configuration like this.

[kzc]

I see what you mean now. Yes, I agree that the various React commonjs modules are not handled well by Rollup. I wonder what Parcel does to enumerate the react exports in this scenario.

[kzc]

To answer my own question, esbuild and parcel do not validate the exports of commonjs modules and will allow the import of invalid symbols - be it a typo or otherwise.

Given a source file with an invalid import:

$ cat test.jsx
import { NO_SUCH_METHOD } from "react";
NO_SUCH_METHOD();

esbuild and parcel will compile it successfully and the error will only be caught at runtime:

$ esbuild test.jsx --bundle --minify --target=es5 | wc -c
   29541

$ node_modules/.bin/parcel build test.jsx --no-cache
✨ Built in 2.72s

dist/test.js    7.29 KB    691ms

whereas Rollup will error out in the compilation phase upon encountering the unknown import:

$ node_modules/.bin/rollup test.jsx --silent -p node-resolve -p sucrase='{transforms:["typescript","jsx"]}' -p commonjs
(!) Error when using sourcemap for reporting an error: Can't resolve original location of error.
test.jsx: (1:9)
[!] Error: 'NO_SUCH_METHOD' is not exported by node_modules/react/index.js, imported by test.jsx
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
test.jsx (1:9)
1: import { NO_SUCH_METHOD } from "react";
            ^
2: NO_SUCH_METHOD();
Error: 'NO_SUCH_METHOD' is not exported by node_modules/react/index.js, imported by test.jsx

So while it's inconvenient to manually enumerate the exports of commonjs modules in some cases, Rollup is just being more cautious. This seems to be a different philosophical approach rather than a bug. It performs more static analysis and error checking upfront, whereas esbuild and parcel defer to flagging errors at runtime for commonjs modules.

But if rollup compiles source code successfully, I'm not aware of a case where it produces incorrect code upon the import of a commonjs module.