Make sure system exports are valid JavaScript (#3960)

Also turn validate into a warning to be able to inspect generated output
rollup · Feb 12, 2021 · 1794a71 · 1794a71
1 parent 6f16528
commit 1794a71
Show file tree

Hide file tree

Showing 16 changed files with 43 additions and 27 deletions.
diff --git a/cli/run/batchWarnings.ts b/cli/run/batchWarnings.ts
@@ -41,7 +41,7 @@ export default function batchWarnings() {
 				const id = (warning.loc && warning.loc.file) || warning.id;
 				if (id) {
 					const loc = warning.loc
-						? `${relativeId(id)}: (${warning.loc.line}:${warning.loc.column})`
+						? `${relativeId(id)} (${warning.loc.line}:${warning.loc.column})`
 						: relativeId(id);
 
 					stderr(bold(relativeId(loc)));

diff --git a/docs/999-big-list-of-options.md b/docs/999-big-list-of-options.md
@@ -931,6 +931,8 @@ Default: `false`
 
 Re-parses each generated chunk to detect if the generated code is valid JavaScript. This can be useful to debug output generated by plugins that use the [`renderChunk`](guide/en/#renderchunk) hook to transform code.
 
+If the code is invalid, a warning will be issued. Note that no error is thrown so that you can still inspect the generated output. To promote this warning to an error, you can watch for it in an [`onwarn`](guide/en/#onwarn) handler.
+
 #### preserveEntrySignatures
 Type: `"strict" | "allow-extension" | "exports-only" | false`<br>
 CLI: `--preserveEntrySignatures <strict|allow-extension>`/`--no-preserveEntrySignatures`<br>

diff --git a/src/Bundle.ts b/src/Bundle.ts
@@ -186,7 +186,7 @@ export default class Bundle {
 						ecmaVersion: 'latest'
 					});
 				} catch (exception) {
-					return error(errChunkInvalid(file, exception));
+					this.inputOptions.onwarn(errChunkInvalid(file, exception));
 				}
 			}
 		}

diff --git a/src/ast/nodes/AssignmentExpression.ts b/src/ast/nodes/AssignmentExpression.ts
@@ -114,9 +114,9 @@ export default class AssignmentExpression extends NodeBase {
 				if (systemPatternExports.length > 0) {
 					code.prependRight(
 						this.start,
-						getSystemExportFunctionLeft(systemPatternExports, true, options)
+						`(${getSystemExportFunctionLeft(systemPatternExports, true, options)}`
 					);
-					code.appendLeft(this.end, ')');
+					code.appendLeft(this.end, '))');
 				}
 			}
 		}

diff --git a/src/ast/nodes/UpdateExpression.ts b/src/ast/nodes/UpdateExpression.ts
@@ -1,9 +1,6 @@
 import MagicString from 'magic-string';
 import { RenderOptions } from '../../utils/renderHelpers';
-import {
-	getSystemExportFunctionLeft,
-	getSystemExportStatement
-} from '../../utils/systemJsRendering';
+import { getSystemExportFunctionLeft, getSystemExportStatement } from '../../utils/systemJsRendering';
 import { HasEffectsContext } from '../ExecutionContext';
 import { EMPTY_PATH, ObjectPath } from '../utils/PathTracker';
 import Identifier from './Identifier';
@@ -65,7 +62,7 @@ export default class UpdateExpression extends NodeBase {
 					code.overwrite(
 						this.start,
 						this.end,
-						`${getSystemExportFunctionLeft([variable!], false, options)}${this.operator}${name})`
+						`(${getSystemExportFunctionLeft([variable!], false, options)}${this.operator}${name}))`
 					);
 				} else {
 					let op;

diff --git a/src/utils/pureComments.ts b/src/utils/pureComments.ts
@@ -1,5 +1,6 @@
 import * as acorn from 'acorn';
 import { base as basicWalker, BaseWalker } from 'acorn-walk';
+import { CallExpression, ExpressionStatement, NewExpression } from '../ast/nodes/NodeType';
 import { CommentDescription } from '../Module';
 
 // patch up acorn-walk until class-fields are officially supported
@@ -13,7 +14,8 @@ basicWalker.PropertyDefinition = function (node: any, st: any, c: any) {
 };
 
 interface CommentState {
-	commentIndex: number; commentNodes: CommentDescription[]
+	commentIndex: number;
+	commentNodes: CommentDescription[];
 }
 
 function handlePureAnnotationsOfNode(
@@ -40,10 +42,10 @@ function markPureNode(
 	} else {
 		node.annotations = [comment];
 	}
-	if (node.type === 'ExpressionStatement') {
+	if (node.type === ExpressionStatement) {
 		node = (node as any).expression;
 	}
-	if (node.type === 'CallExpression' || node.type === 'NewExpression') {
+	if (node.type === CallExpression || node.type === NewExpression) {
 		(node as any).annotatedPure = true;
 	}
 }

diff --git a/test/cli/samples/validate/_config.js b/test/cli/samples/validate/_config.js
@@ -8,7 +8,7 @@ module.exports = {
 	stderr: stderr =>
 		assertIncludes(
 			stderr,
-			`[!] Error: Chunk "out.js" is not valid JavaScript: Unterminated comment (3:20).
+			`(!) Chunk "out.js" is not valid JavaScript: Unterminated comment (3:20).
 out.js (3:20)
 1: console.log(2 );
 2: 

diff --git a/test/cli/samples/warn-multiple/_config.js b/test/cli/samples/warn-multiple/_config.js
@@ -25,7 +25,7 @@ module.exports = {
 			stderr,
 
 			"(!) Module level directives cause errors when bundled, 'use stuff' was ignored.\n" +
-				'main.js: (1:0)\n' +
+				'main.js (1:0)\n' +
 				"1: 'use stuff';\n" +
 				'   ^\n' +
 				'2: \n' +

diff --git a/test/form/index.js b/test/form/index.js
@@ -14,6 +14,7 @@ runTestSuiteWithSamples('form', path.resolve(__dirname, 'samples'), (dir, config
 		() => {
 			let bundle;
 			const runRollupTest = async (inputFile, bundleFile, defaultFormat) => {
+				const warnings = [];
 				if (config.before) config.before();
 				try {
 					process.chdir(dir);
@@ -30,10 +31,7 @@ runTestSuiteWithSamples('form', path.resolve(__dirname, 'samples'), (dir, config
 												config.expectedWarnings.indexOf(warning.code) >= 0
 											)
 										) {
-											throw new Error(
-												`Unexpected warnings (${warning.code}): ${warning.message}\n` +
-													'If you expect warnings, list their codes in config.expectedWarnings'
-											);
+											warnings.push(warning);
 										}
 									},
 									strictDeprecations: true
@@ -48,7 +46,7 @@ runTestSuiteWithSamples('form', path.resolve(__dirname, 'samples'), (dir, config
 								exports: 'auto',
 								file: inputFile,
 								format: defaultFormat,
-								// validate: true // add when systemjs bugs fixed
+								validate: true
 							},
 							(config.options || {}).output || {}
 						),
@@ -58,6 +56,17 @@ runTestSuiteWithSamples('form', path.resolve(__dirname, 'samples'), (dir, config
 				} finally {
 					if (config.after) config.after();
 				}
+				if (warnings.length > 0) {
+					const codes = new Set();
+					for (const { code } of warnings) {
+						codes.add(code);
+					}
+					throw new Error(
+						`Unexpected warnings (${[...codes].join(', ')}): \n${warnings
+							.map(({ message }) => `${message}\n\n`)
+							.join('')}` + 'If you expect warnings, list their codes in config.expectedWarnings'
+					);
+				}
 			};
 
 			if (isSingleFormatTest) {

diff --git a/test/form/samples/system-export-compact/_expected.js b/test/form/samples/system-export-compact/_expected.js
@@ -1,3 +1,3 @@
 System.register([],function(exports){'use strict';return{execute:function(){exports({a:void 0,b:void 0,c:void 0});let a, b;
 
-function(v){return exports({a:a}),v}([{ b: a = (exports('a',a-1),a--) }] = { b: function(v){return exports({b:v,c:v}),v}(--b) });}}});
+(function(v){return exports({a:a}),v}([{ b: a = (exports('a',a-1),a--) }] = { b: (function(v){return exports({b:v,c:v}),v}(--b)) })), (function(v){return exports({a:a}),v}([a] = [1]));}}});
diff --git a/test/form/samples/system-export-compact/main.js b/test/form/samples/system-export-compact/main.js
@@ -1,5 +1,5 @@
 export let a, b;
 
-[{ b: a = a-- }] = { b: b-- };
+[{ b: a = a-- }] = { b: b-- }, [a] = [1];
 
-export { b as c}
+export { b as c };
diff --git a/test/form/samples/system-export-destructuring-assignment/_expected.js b/test/form/samples/system-export-destructuring-assignment/_expected.js
@@ -11,8 +11,8 @@ System.register([], function (exports) {
 
 			let a, b, c;
 
-			console.log(function (v) { return exports({ a: a }), v; }({a} = someObject));
-			(function (v) { return exports({ b: b, c: c }), v; }({b, c} = someObject));
+			console.log((function (v) { return exports({ a: a }), v; }({a} = someObject)));
+			((function (v) { return exports({ b: b, c: c }), v; }({b, c} = someObject)));
 
 		}
 	};

diff --git a/test/form/samples/system-multiple-export-bindings/_expected.js b/test/form/samples/system-multiple-export-bindings/_expected.js
@@ -30,12 +30,12 @@ System.register([], function (exports) {
 			a = function (v) { return exports({ a: v, a2: v }), v; }(b = exports('b', c = function (v) { return exports({ c: v, c2: v }), v; }(0)));
 
 			// Destructing Assignment Expression
-			(function (v) { return exports({ a: a, a2: a, b: b, c: c, c2: c }), v; }({ a, b, c } = { c: 4, b: 5, a: 6 }));
+			((function (v) { return exports({ a: a, a2: a, b: b, c: c, c2: c }), v; }({ a, b, c } = { c: 4, b: 5, a: 6 })));
 
 			// Destructuring Defaults
 			var p = function (v) { return exports({ p: v, pp: v }), v; }(5);
 			var q = function (v) { return exports({ q: v, qq: v }), v; }(10);
-			(function (v) { return exports({ p: p, pp: p }), v; }({ p = q = function (v) { return exports({ q: v, qq: v }), v; }(20) } = {}));
+			((function (v) { return exports({ p: p, pp: p }), v; }({ p = q = function (v) { return exports({ q: v, qq: v }), v; }(20) } = {})));
 
 			// Function Assignment
 			function fn () {
@@ -44,7 +44,8 @@ System.register([], function (exports) {
 			fn = function (v) { return exports({ fn: v, fn2: v }), v; }(5);
 
 			// Update Expression
-			function (v) { return exports({ a: v, a2: v }), v; }(++a), (exports('b', b + 1), b++), (++c, exports({ c: c, c2: c }), c);
+			(function (v) { return exports({ a: v, a2: v }), v; }(++a)), (exports('b', b + 1), b++), (++c, exports({ c: c, c2: c }), c);
+			(function (v) { return exports({ a: v, a2: v }), v; }(++a));
 
 			// Class Declaration
 			class A {} exports({ A: A, B: A });

diff --git a/test/form/samples/system-multiple-export-bindings/main.js b/test/form/samples/system-multiple-export-bindings/main.js
@@ -31,6 +31,7 @@ export { fn as fn2 }
 
 // Update Expression
 a++, b++, ++c;
+true && a++;
 
 // Class Declaration
 export class A {} 

diff --git a/test/function/samples/validate-output/_config.js b/test/function/samples/validate-output/_config.js
@@ -1,6 +1,9 @@
 module.exports = {
 	description: 'handles validate failure',
 	options: {
+		onwarn(warning) {
+			throw warning;
+		},
 		output: {
 			outro: '/*',
 			validate: true

diff --git a/test/utils.js b/test/utils.js
@@ -17,6 +17,7 @@ exports.assertIncludes = assertIncludes;
 
 function normaliseError(error) {
 	delete error.stack;
+	delete error.toString;
 	return Object.assign({}, error, {
 		message: error.message
 	});