From d759a98689cd2f71e21f0e08b2a7bab7ef56cf32 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 12 Feb 2021 16:48:36 +0100 Subject: [PATCH] Make sure system exports are valid JavaScript Also turn validate into a warning to be able to inspect generated output --- cli/run/batchWarnings.ts | 2 +- docs/999-big-list-of-options.md | 2 ++ src/Bundle.ts | 2 +- src/ast/nodes/AssignmentExpression.ts | 4 ++-- src/ast/nodes/UpdateExpression.ts | 7 ++----- src/utils/pureComments.ts | 8 +++++--- test/cli/samples/validate/_config.js | 2 +- test/cli/samples/warn-multiple/_config.js | 2 +- test/form/index.js | 19 ++++++++++++++----- .../system-export-compact/_expected.js | 2 +- .../samples/system-export-compact/main.js | 4 ++-- .../_expected.js | 4 ++-- .../_expected.js | 7 ++++--- .../system-multiple-export-bindings/main.js | 1 + .../samples/validate-output/_config.js | 3 +++ test/utils.js | 1 + 16 files changed, 43 insertions(+), 27 deletions(-) diff --git a/cli/run/batchWarnings.ts b/cli/run/batchWarnings.ts index 1fe3d03ae30..508c65e325a 100644 --- 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 index d70ca194b8e..a8aa3408043 100755 --- 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`
CLI: `--preserveEntrySignatures `/`--no-preserveEntrySignatures`
diff --git a/src/Bundle.ts b/src/Bundle.ts index 21afff03c38..65f97532c52 100644 --- 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 index 8dcd6f4fd0c..f982147be34 100644 --- 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 index 174606a2124..2d2a60f621d 100644 --- 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 index 671e590b913..7c2db33d0ee 100644 --- 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 index 934ca67c2a5..5ac726397c6 100644 --- 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 index 0e65b34dd15..3b3474524de 100644 --- 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 index dc5b34e9e7b..1a712e550cb 100644 --- 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 index 92a72b3a664..1160f5e7b11 100644 --- 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) });}}}); \ No newline at end of file +(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]));}}}); \ No newline at end of file diff --git a/test/form/samples/system-export-compact/main.js b/test/form/samples/system-export-compact/main.js index f55377e6247..41f58664a03 100644 --- 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 index 597635361da..37d18c89069 100644 --- 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 index 468098dc162..c72502e8b94 100644 --- 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 index 7e3ed623698..87927a19c0f 100644 --- 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 index 9850e118ef0..1dff84eda97 100644 --- 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 index 9b1610583e5..f807df54e30 100644 --- 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 });