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

Invalid code generated from large minified file #2684

Closed
thgh opened this issue Feb 9, 2019 · 6 comments · Fixed by #2696
Closed

Invalid code generated from large minified file #2684

thgh opened this issue Feb 9, 2019 · 6 comments · Fixed by #2696

Comments

@thgh
Copy link

thgh commented Feb 9, 2019

  • Rollup Version: 1.1.2 (also tried 1.0.0, 1.0.2, 1.1.0)
  • Operating System (or Browser): macos + Chrome (same for cli + api + rollup.browser.es.js)
  • Node Version: 11.2.0 (same output in deno0.2.11, repl)

How Do We Reproduce?

repo: rollup-treeshake-issue
input: prettier/vendor/parser_typescript.js

I'm importing a minified 2Mb js file without dependencies. With default settings, the output is invalid. When using treeshake: false, the output is valid.

input: 2 173 862 bytes => syntax ok
output: 2 167 342 bytes => syntax error near 3rd occurence of "emitDecoratorMetadata"
error message when executing output: Unexpected token (11:1264563)

Expected Behavior

Rollup outputs valid code without syntax errors.

Actual Behavior

Syntax error in output file on line 11 column 1264563. 🙄

A closer look

Notice that the functions on line 5&6 don't have a name (expected identifier)

function ee(r, i, a) {
  if (a) {
    var s = [];
    return e.addRange(s, e.map(a.decorators, ne)), e.addRange(s, e.flatMap(a.parameters, ie)),
      function(r, i, a) {
        function(e, r, n) {
          D.emitDecoratorMetadata &&
            (ae(e) && n.push(o(t, "design:type", ce(e))), se(e) && n.push(o(t, "design:paramtypes", ue(e, r))), oe(e) && n.push(o(t, "design:returntype", le(e))));
        }(r, i, a);
      }(r, i, s), s
  }
}
@kzc
Copy link
Contributor

kzc commented Feb 10, 2019

Rollup doesn't emit parens for ternary consequent and alternate.

$ cat test.js
1 ? function foo(){ console.log("consequent"); }() : 2;
0 ? 1 : function(){ console.log("alternate"); }();
$ cat test.js | node
consequent
alternate
$ node_modules/.bin/rollup test.js --silent -f es
function foo(){ console.log("consequent"); }();
function(){ console.log("alternate"); }();
$ node_modules/.bin/rollup test.js --silent -f es | node
[stdin]:1
function foo(){ console.log("consequent"); }();
                                             ^
SyntaxError: Unexpected token )

@kzc
Copy link
Contributor

kzc commented Feb 10, 2019

An amusing variation that produces syntactically valid but incorrect code:

$ cat t2.js
function foo(){ console.log("foo"); }
function bar(){ console.log("bar"); }
1 ? function foo(x){ console.log(x); }("consequent") : 2;
0 ? 1 : function bar(x){ console.log(x); }("alternate");
foo("hello");
bar("world");
$ cat t2.js | node
consequent
alternate
foo
bar
$ node_modules/.bin/rollup t2.js -f es --silent
function foo(){ console.log("foo"); }
function bar(){ console.log("bar"); }
function foo(x){ console.log(x); }("consequent");
function bar(x){ console.log(x); }("alternate");
foo("hello");
bar("world");
$ node_modules/.bin/rollup t2.js -f es --silent | node
hello
world

@kzc
Copy link
Contributor

kzc commented Feb 10, 2019

&& and || operators have the same problem as the ternary operator:

$ cat t3.js
function foo(){ console.log("foo"); }
function bar(){ console.log("bar"); }
0 || function foo(x){ console.log(x); }("or");
1 && function bar(x){ console.log(x); }("and");
foo("wrong");
bar("result");
$ cat t3.js | node
or
and
foo
bar
$ node_modules/.bin/rollup t3.js -f es --silent
function foo(){ console.log("foo"); }
function bar(){ console.log("bar"); }
function foo(x){ console.log(x); }("or");
function bar(x){ console.log(x); }("and");
foo("wrong");
bar("result");
$ node_modules/.bin/rollup t3.js -f es --silent | node
wrong
result

@kzc
Copy link
Contributor

kzc commented Feb 10, 2019

The comma operator has the same issue:

$ cat t4.js
function foo(){ console.log("foo"); }
0, function foo(x){ console.log(x); }("comma");
foo("incorrect");
$ cat t4.js | node
comma
foo
$ node_modules/.bin/rollup t4.js -f es --silent
function foo(){ console.log("foo"); }
function foo(x){ console.log(x); }("comma");
foo("incorrect");
$ node_modules/.bin/rollup t4.js -f es --silent | node
incorrect

@lukastaegert
Copy link
Member

Fix at #2696. I can confirm that this also resolves the original issue provided one allocates more than the default amount of memory to node when running acorn.

It turned out the issue was surprisingly easy to fix as the necessary concepts had already been introduced to Rollup's code quite some time ago to handle some other edge cases. If you can find more issues of this kind, please let us know!

@thgh
Copy link
Author

thgh commented Feb 18, 2019

@kzc Impressive troubleshooting!
@lukastaegert thanks for the quick fix!

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

Successfully merging a pull request may close this issue.

3 participants