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

Make sure system exports are valid JavaScript #3960

Merged
merged 1 commit into from Feb 12, 2021
Merged

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Feb 12, 2021

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.

Also turn validate into a warning to be able to inspect generated output
@rollup-bot
Copy link
Collaborator

@rollup-bot rollup-bot commented Feb 12, 2021

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
Copy link

@codecov codecov bot commented Feb 12, 2021

Codecov Report

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

Impacted file tree graph

@@            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.

@lukastaegert lukastaegert merged commit 1794a71 into master Feb 12, 2021
9 checks passed
9 checks passed
@github-actions
14 (Windows)
Details
@github-actions
12 (Windows)
Details
@github-actions
10 (Windows)
Details
ci/circleci: analysis Your tests passed on CircleCI!
Details
ci/circleci: node-v10-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v12-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v14-latest Your tests passed on CircleCI!
Details
@codecov
codecov/patch 100.00% of diff hit (target 97.19%)
Details
@codecov
codecov/project 97.21% (+0.01%) compared to 6f16528
Details
@lukastaegert lukastaegert deleted the wrap-system-exports branch Feb 12, 2021
@kzc
Copy link
Contributor

@kzc kzc commented Feb 12, 2021

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
Copy link
Contributor

@kzc kzc commented Feb 12, 2021

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
Copy link
Member Author

@lukastaegert lukastaegert commented Feb 12, 2021

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

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Feb 12, 2021

@kzc
Copy link
Contributor

@kzc kzc commented Feb 12, 2021

@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
Copy link
Contributor

@kzc kzc commented Feb 12, 2021

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
Copy link
Contributor

@kzc kzc commented Feb 12, 2021

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
Copy link
Contributor

@kzc kzc commented Feb 13, 2021

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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants