Make sure system exports are valid JavaScript

[lukastaegert]

Also turn validate into a warning to be able to inspect generated output

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

Description

As prompted by #3952, this wraps certain System export related expressions in parentheses to avoid invalid code. Also, this changes the validate option from throwing an error to emitting a warning to allow inspecting the generated output manually.

[rollup-bot]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#wrap-system-exports

or load it into the REPL:
https://rollupjs.org/repl/?circleci=14309

[codecov]

Codecov Report

Merging #3960 (d759a98) into master (6f16528) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3960      +/-   ##
==========================================
+ Coverage   97.19%   97.21%   +0.01%     
==========================================
  Files         191      191              
  Lines        6703     6704       +1     
  Branches     1960     1960              
==========================================
+ Hits         6515     6517       +2     
  Misses         99       99              
+ Partials       89       88       -1     

Impacted Files	Coverage Δ
src/ast/nodes/UpdateExpression.ts	100.00% <ø> (ø)
cli/run/batchWarnings.ts	98.44% <100.00%> (ø)
src/Bundle.ts	100.00% <100.00%> (ø)
src/ast/nodes/AssignmentExpression.ts	100.00% <100.00%> (ø)
src/utils/pureComments.ts	100.00% <100.00%> (ø)
src/ast/nodes/NodeType.ts	100.00% <0.00%> (ø)
cli/run/build.ts	92.10% <0.00%> (+2.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f16528...d759a98. Read the comment docs.

[kzc]

Also, this changes the validate option from throwing an error to emitting a warning to allow inspecting the generated output manually.

@lukastaegert I was hoping the code frame showing the error would be enough, but I appreciate it may be convenient to inspect the invalid code after a --validate failure. However, I think it would be better to error out in this situation given (1) it's an explicit opt-in and (2) it's a serious failure. I generally spawn rollup from a Makefile without a rollup config with a --silent flag, so a custom onWarn handler is awkward from the CLI.

Is there a simple way to promote a warning to an error from the CLI while still retaining the output files? Perhaps this error could be deferred until after all the files have been emitted?

A good compromise would be to change the option to allow:

--validate to validate with warnings emitted and output files retained.
--validate=error to error out upon first validation failure.
--validate=warning which would be the same as --validate.

[kzc]

I see that --silent does not silence a --validate failure, which is good:

$ echo 'console.log(1 + 2)' | rollup --outro '/*' --silent --validate -o out.mjs && echo SUCCESS
(!) Chunk "out.mjs" is not valid JavaScript: Unterminated comment (3:0).
out.mjs (3:0)
1: console.log(1 + 2);
2: 
3: /*
   ^
SUCCESS

But the rollup process returning a zero error code upon --validate failure is not ideal for Makefiles and other similar build tools.

[lukastaegert]

You want —failAfterWarnings. The chunking form tests should also error for unexpected warnings, just like the form tests.

[lukastaegert]

https://rollupjs.org/guide/en/#--failafterwarnings

[kzc]

@lukastaegert Thanks for the quick reply. Unfortunately --failAfterWarnings is not working correctly:

$ rollup -v
rollup v2.39.0

$ node -v
v14.3.0

Without -—failAfterWarnings - zero exit code as expected:

$ echo 'console.log(1?2:3)' | rollup --silent --outro '/*' --validate && echo SUCCESS
(!) Chunk "-.js" is not valid JavaScript: Unterminated comment (3:0).
-.js (3:0)
1: console.log(2);
2: 
3: /*
   ^
console.log(2);

/*
SUCCESS

With -—failAfterWarnings - an unexpected error with a zero exit code indicating success:

$ echo 'console.log(1?2:3)' | rollup --silent --outro '/*' --validate -—failAfterWarnings && echo SUCCESS
(node:23952) UnhandledPromiseRejectionWarning: TypeError: input.indexOf is not a function
    at /opt/rollup/rollup/cli/run/index.ts:28:49
    at Array.some (<anonymous>)
...
(Use `node --trace-warnings ...` to show where the warning was created)
(node:23952) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:23952) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
SUCCESS

[kzc]

Wow... this is the weirdest thing. I had cut and pasted the option from #3960 (comment) and prepended the missing hyphen to get the error above.

But when I manually retyped it - and it's seemingly identical - it does work:

$ echo 'console.log(1?2:3)' | rollup --silent --outro '/*' --validate --failAfterWarnings && echo SUCCESS
(!) Chunk "-.js" is not valid JavaScript: Unterminated comment (3:0).
-.js (3:0)
1: console.log(2);
2: 
3: /*
   ^
console.log(2);

/*
[!] Warnings occurred and --failAfterWarnings flag present

There was a strange character in #3960 (comment) causing the problem:

$ echo '—failAfterWarnings' | od -c
0000000    —  **  **   f   a   i   l   A   f   t   e   r   W   a   r   n
0000020    i   n   g   s  \n                                            
0000025

That's nuts.

Luckily, this unknown invisible character is not present when copy/pasted from the official docs in https://rollupjs.org/guide/en/#--failafterwarnings.

$ echo '--failAfterWarnings' | od -c
0000000    -   -   f   a   i   l   A   f   t   e   r   W   a   r   n   i
0000020    n   g   s  \n                                                

No rollup bug.

[kzc]

Cursed unicode character 'EM DASH' (U+2014)!

$ echo '-—failAfterWarnings' | od -c -t x1
0000000    -   —  **  **   f   a   i   l   A   f   t   e   r   W   a   r
           2d  e2  80  94  66  61  69  6c  41  66  74  65  72  57  61  72
0000020    n   i   n   g   s  \n                                        
           6e  69  6e  67  73  0a                                        
0000026

I can't be the only one bit by this. yargs-parser ought to check for it.

[kzc]

One could pathologically use EM DASH as a short flag like -i, so it's not technically a yargs-parser issue.

In any case, this small patch could prevent such accidental CLI misuse of EM DASH and avoid hard to debug errors for others:

--- a/cli/cli.ts
+++ b/cli/cli.ts
@@ -9,6 +9,11 @@ const command = argParser(process.argv.slice(2), {
        configuration: { 'camel-case-expansion': false }
 });
 
+if ("\u2014" /* EM DASH */ in command || "\u2E3A" /* TWO-EM DASH */ in command) {
+       console.error("Error: Hyphen-like character used instead of hyphen in command argument");
+       process.exit(1);
+}
+
 if (command.help || (process.argv.length <= 2 && process.stdin.isTTY)) {
        console.log(`\n${help.replace('__VERSION__', version)}\n`);
 } else if (command.version) {

without patch:

$ echo 'console.log(1?2:3)' | rollup --silent --outro '/*' --validate -—failAfterWarnings && echo SUCCESS || echo FAIL
(node:23952) UnhandledPromiseRejectionWarning: TypeError: input.indexOf is not a function
    at /opt/rollup/rollup/cli/run/index.ts:28:49
    at Array.some (<anonymous>)
...
(Use `node --trace-warnings ...` to show where the warning was created)
(node:23952) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:23952) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
SUCCESS

with patch:

$ echo 'console.log(1?2:3)' | rollup --silent --outro '/*' --validate -—failAfterWarnings && echo SUCCESS || echo FAIL
Error: Hyphen-like character used instead of hyphen in command argument
FAIL

Note: EM-DASH characters can still be used as arguments to other CLI options with the patch:

$ echo 'console.log(1?2:3)' | rollup --silent --outro '/* EM—DASH used here */'
console.log(2);

/* EM—DASH used here */

$ echo 'console.log(1?2:3)' | rollup --silent --outro '/* EM—DASH used here */' | od -ct x1
0000000    c   o   n   s   o   l   e   .   l   o   g   (   2   )   ;  \n
           63  6f  6e  73  6f  6c  65  2e  6c  6f  67  28  32  29  3b  0a
0000020   \n   /   *       E   M   —  **  **   D   A   S   H       u   s
           0a  2f  2a  20  45  4d  e2  80  94  44  41  53  48  20  75  73
0000040    e   d       h   e   r   e       *   /  \n                    
           65  64  20  68  65  72  65  20  2a  2f  0a                    
0000053

Edit: the patch was updated to put the check before version and help processing.