From 47d3daf9a5275e35492e7b3552cfc62838234fff Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 07:56:54 -0500 Subject: [PATCH 01/28] statically analyse LogicalExpression nodes (#1061) --- src/ast/nodes/LogicalExpression.js | 19 +++++++++++++++++++ src/ast/nodes/index.js | 2 ++ test/form/skips-dead-branches-i/_config.js | 2 +- test/form/skips-dead-branches-j/_config.js | 3 +++ .../skips-dead-branches-j/_expected/amd.js | 7 +++++++ .../skips-dead-branches-j/_expected/cjs.js | 5 +++++ .../skips-dead-branches-j/_expected/es.js | 3 +++ .../skips-dead-branches-j/_expected/iife.js | 8 ++++++++ .../skips-dead-branches-j/_expected/umd.js | 11 +++++++++++ test/form/skips-dead-branches-j/main.js | 5 +++++ 10 files changed, 64 insertions(+), 1 deletion(-) create mode 100644 src/ast/nodes/LogicalExpression.js create mode 100644 test/form/skips-dead-branches-j/_config.js create mode 100644 test/form/skips-dead-branches-j/_expected/amd.js create mode 100644 test/form/skips-dead-branches-j/_expected/cjs.js create mode 100644 test/form/skips-dead-branches-j/_expected/es.js create mode 100644 test/form/skips-dead-branches-j/_expected/iife.js create mode 100644 test/form/skips-dead-branches-j/_expected/umd.js create mode 100644 test/form/skips-dead-branches-j/main.js diff --git a/src/ast/nodes/LogicalExpression.js b/src/ast/nodes/LogicalExpression.js new file mode 100644 index 0000000..4f81d3b --- /dev/null +++ b/src/ast/nodes/LogicalExpression.js @@ -0,0 +1,19 @@ +import Node from '../Node.js'; +import { UNKNOWN } from '../values.js'; + +const operators = { + '&&': ( left, right ) => left && right, + '||': ( left, right ) => left || right +}; + +export default class LogicalExpression extends Node { + getValue () { + const leftValue = this.left.getValue(); + if ( leftValue === UNKNOWN ) return UNKNOWN; + + const rightValue = this.right.getValue(); + if ( rightValue === UNKNOWN ) return UNKNOWN; + + return operators[ this.operator ]( leftValue, rightValue ); + } +} diff --git a/src/ast/nodes/index.js b/src/ast/nodes/index.js index 1c3ba4c..2c59ee4 100644 --- a/src/ast/nodes/index.js +++ b/src/ast/nodes/index.js @@ -21,6 +21,7 @@ import Identifier from './Identifier.js'; import IfStatement from './IfStatement.js'; import ImportDeclaration from './ImportDeclaration.js'; import Literal from './Literal.js'; +import LogicalExpression from './LogicalExpression.js'; import MemberExpression from './MemberExpression.js'; import NewExpression from './NewExpression.js'; import ObjectExpression from './ObjectExpression.js'; @@ -59,6 +60,7 @@ export default { IfStatement, ImportDeclaration, Literal, + LogicalExpression, MemberExpression, NewExpression, ObjectExpression, diff --git a/test/form/skips-dead-branches-i/_config.js b/test/form/skips-dead-branches-i/_config.js index 85e8fbe..95aa942 100644 --- a/test/form/skips-dead-branches-i/_config.js +++ b/test/form/skips-dead-branches-i/_config.js @@ -1,3 +1,3 @@ module.exports = { - description: 'skips a dead branch (h)' + description: 'skips a dead branch (i)' }; diff --git a/test/form/skips-dead-branches-j/_config.js b/test/form/skips-dead-branches-j/_config.js new file mode 100644 index 0000000..b9abe10 --- /dev/null +++ b/test/form/skips-dead-branches-j/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'skips a dead branch (j)' +}; diff --git a/test/form/skips-dead-branches-j/_expected/amd.js b/test/form/skips-dead-branches-j/_expected/amd.js new file mode 100644 index 0000000..9ee9589 --- /dev/null +++ b/test/form/skips-dead-branches-j/_expected/amd.js @@ -0,0 +1,7 @@ +define(function () { 'use strict'; + + { + console.log( 'true' ); + } + +}); diff --git a/test/form/skips-dead-branches-j/_expected/cjs.js b/test/form/skips-dead-branches-j/_expected/cjs.js new file mode 100644 index 0000000..f71d9f9 --- /dev/null +++ b/test/form/skips-dead-branches-j/_expected/cjs.js @@ -0,0 +1,5 @@ +'use strict'; + +{ + console.log( 'true' ); +} diff --git a/test/form/skips-dead-branches-j/_expected/es.js b/test/form/skips-dead-branches-j/_expected/es.js new file mode 100644 index 0000000..bbd290a --- /dev/null +++ b/test/form/skips-dead-branches-j/_expected/es.js @@ -0,0 +1,3 @@ +{ + console.log( 'true' ); +} diff --git a/test/form/skips-dead-branches-j/_expected/iife.js b/test/form/skips-dead-branches-j/_expected/iife.js new file mode 100644 index 0000000..c506520 --- /dev/null +++ b/test/form/skips-dead-branches-j/_expected/iife.js @@ -0,0 +1,8 @@ +(function () { + 'use strict'; + + { + console.log( 'true' ); + } + +}()); diff --git a/test/form/skips-dead-branches-j/_expected/umd.js b/test/form/skips-dead-branches-j/_expected/umd.js new file mode 100644 index 0000000..2f6e70a --- /dev/null +++ b/test/form/skips-dead-branches-j/_expected/umd.js @@ -0,0 +1,11 @@ +(function (global, factory) { + typeof exports === 'object' && typeof module !== 'undefined' ? factory() : + typeof define === 'function' && define.amd ? define(factory) : + (factory()); +}(this, (function () { 'use strict'; + + { + console.log( 'true' ); + } + +}))); diff --git a/test/form/skips-dead-branches-j/main.js b/test/form/skips-dead-branches-j/main.js new file mode 100644 index 0000000..afc8c67 --- /dev/null +++ b/test/form/skips-dead-branches-j/main.js @@ -0,0 +1,5 @@ +if ( true && true ) { + console.log( 'true' ); +} else { + console.log( 'false' ); +} From c36f965e7de5605e8931c1d113628e12890f3825 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 08:28:39 -0500 Subject: [PATCH 02/28] warn about using node built-ins in browser bundle (#1051) --- src/finalisers/amd.js | 2 ++ src/finalisers/iife.js | 4 ++- src/finalisers/shared/warnOnBuiltins.js | 39 +++++++++++++++++++++++++ src/finalisers/umd.js | 3 ++ test/test.js | 29 ++++++++++++++++-- 5 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 src/finalisers/shared/warnOnBuiltins.js diff --git a/src/finalisers/amd.js b/src/finalisers/amd.js index 70b404d..efe1a8e 100644 --- a/src/finalisers/amd.js +++ b/src/finalisers/amd.js @@ -2,8 +2,10 @@ import { getName, quotePath } from '../utils/map-helpers.js'; import getInteropBlock from './shared/getInteropBlock.js'; import getExportBlock from './shared/getExportBlock.js'; import esModuleExport from './shared/esModuleExport.js'; +import warnOnBuiltins from './shared/warnOnBuiltins.js'; export default function amd ( bundle, magicString, { exportMode, indentString, intro, outro }, options ) { + warnOnBuiltins( bundle ); const deps = bundle.externalModules.map( quotePath ); const args = bundle.externalModules.map( getName ); diff --git a/src/finalisers/iife.js b/src/finalisers/iife.js index 60105c1..f6d702e 100644 --- a/src/finalisers/iife.js +++ b/src/finalisers/iife.js @@ -4,6 +4,7 @@ import getInteropBlock from './shared/getInteropBlock.js'; import getExportBlock from './shared/getExportBlock.js'; import getGlobalNameMaker from './shared/getGlobalNameMaker.js'; import propertyStringFor from './shared/propertyStringFor'; +import warnOnBuiltins from './shared/warnOnBuiltins.js'; // thisProp('foo.bar-baz.qux') === "this.foo['bar-baz'].qux" const thisProp = propertyStringFor('this'); @@ -29,8 +30,9 @@ export default function iife ( bundle, magicString, { exportMode, indentString, const name = options.moduleName; const isNamespaced = name && ~name.indexOf( '.' ); - const dependencies = bundle.externalModules.map( globalNameMaker ); + warnOnBuiltins( bundle ); + const dependencies = bundle.externalModules.map( globalNameMaker ); const args = bundle.externalModules.map( getName ); if ( exportMode !== 'none' && !name ) { diff --git a/src/finalisers/shared/warnOnBuiltins.js b/src/finalisers/shared/warnOnBuiltins.js new file mode 100644 index 0000000..e5ac1ba --- /dev/null +++ b/src/finalisers/shared/warnOnBuiltins.js @@ -0,0 +1,39 @@ +const builtins = { + process: true, + events: true, + stream: true, + util: true, + path: true, + buffer: true, + querystring: true, + url: true, + string_decoder: true, + punycode: true, + http: true, + https: true, + os: true, + assert: true, + constants: true, + timers: true, + console: true, + vm: true, + zlib: true, + tty: true, + domain: true +}; + +// Creating a browser bundle that depends on Node.js built-in modules ('util'). You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins + +export default function warnOnBuiltins ( bundle ) { + const externalBuiltins = bundle.externalModules + .filter( mod => mod.id in builtins ) + .map( mod => mod.id ); + + if ( !externalBuiltins.length ) return; + + const detail = externalBuiltins.length === 1 ? + `module ('${externalBuiltins[0]}')` : + `modules (${externalBuiltins.slice( 0, -1 ).map( name => `'${name}'` ).join( ', ' )} and '${externalBuiltins.pop()}')`; + + bundle.onwarn( `Creating a browser bundle that depends on Node.js built-in ${detail}. You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins` ); +} diff --git a/src/finalisers/umd.js b/src/finalisers/umd.js index 279226d..c176242 100644 --- a/src/finalisers/umd.js +++ b/src/finalisers/umd.js @@ -5,6 +5,7 @@ import getExportBlock from './shared/getExportBlock.js'; import getGlobalNameMaker from './shared/getGlobalNameMaker.js'; import esModuleExport from './shared/esModuleExport.js'; import propertyStringFor from './shared/propertyStringFor.js'; +import warnOnBuiltins from './shared/warnOnBuiltins.js'; // globalProp('foo.bar-baz') === "global.foo['bar-baz']" const globalProp = propertyStringFor('global'); @@ -30,6 +31,8 @@ export default function umd ( bundle, magicString, { exportMode, indentString, i throw new Error( 'You must supply options.moduleName for UMD bundles' ); } + warnOnBuiltins( bundle ); + const globalNameMaker = getGlobalNameMaker( options.globals || blank(), bundle.onwarn ); const amdDeps = bundle.externalModules.map( quotePath ); diff --git a/test/test.js b/test/test.js index 8b2b66f..50f63e8 100644 --- a/test/test.js +++ b/test/test.js @@ -49,7 +49,7 @@ function loadConfig ( path ) { function loader ( modules ) { return { resolveId ( id ) { - return id; + return id in modules ? id : null; }, load ( id ) { @@ -710,8 +710,6 @@ describe( 'rollup', function () { dest, format: 'cjs' }); - - }).then( () => { assert.deepEqual( result, [ { a: dest, format: 'cjs' }, @@ -722,4 +720,29 @@ describe( 'rollup', function () { }); }); }); + + describe( 'misc', () => { + it( 'warns if node builtins are unresolved in a non-CJS, non-ES bundle (#1051)', () => { + const warnings = []; + + return rollup.rollup({ + entry: 'entry', + plugins: [ + loader({ entry: `import { format } from 'util';\nexport default format( 'this is a %s', 'formatted string' );` }) + ], + onwarn: warning => warnings.push( warning ) + }).then( bundle => { + bundle.generate({ + format: 'iife', + moduleName: 'myBundle' + }); + + assert.deepEqual( warnings, [ + `'util' is imported by entry, but could not be resolved – treating it as an external dependency. For help see https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency`, + `Creating a browser bundle that depends on Node.js built-in module ('util'). You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins`, + `No name was provided for external module 'util' in options.globals – guessing 'util'` + ]); + }); + }); + }); }); From f8a09fd0bd576a37ae3c5147c6502e5701b151d9 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 09:04:01 -0500 Subject: [PATCH 03/28] -> v0.38.3 --- CHANGELOG.md | 6 ++++++ package.json | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9408986..51eca14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # rollup changelog +## 0.38.3 + +* More informative warning for implicit external dependencies ([#1051](https://github.com/rollup/rollup/issues/1051)) +* Warn when creating browser bundle with external dependencies on Node built-ins ([#1051](https://github.com/rollup/rollup/issues/1051)) +* Statically analyse LogicalExpression nodes, for better dead code removal ([#1061](https://github.com/rollup/rollup/issues/1061)) + ## 0.38.2 * Preserve `var` declarations in dead branches ([#997](https://github.com/rollup/rollup/issues/997)) diff --git a/package.json b/package.json index 76ee18e..f7af247 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollup", - "version": "0.38.2", + "version": "0.38.3", "description": "Next-generation ES6 module bundler", "main": "dist/rollup.js", "module": "dist/rollup.es.js", From 5360abdb3179695fbb8426e7e6cb8ca32d11cc75 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 12:41:25 -0500 Subject: [PATCH 04/28] implement bundle.warn and module.warn, to replace direct calls to bundle.onwarn (#1194) --- package.json | 1 + src/Bundle.js | 5 +++ src/Module.js | 29 +++++++++++--- src/ast/Node.js | 4 +- src/ast/nodes/CallExpression.js | 10 +++-- src/ast/nodes/ExportDefaultDeclaration.js | 4 +- src/ast/nodes/MemberExpression.js | 4 +- src/ast/nodes/ThisExpression.js | 4 +- .../shared/disallowIllegalReassignment.js | 6 +-- src/utils/getCodeFrame.js | 38 +++++++++++++++++++ src/utils/getLocation.js | 20 ---------- .../cannot-call-external-namespace/_config.js | 2 +- .../cannot-call-internal-namespace/_config.js | 2 +- .../duplicate-import-fails/_config.js | 4 +- .../_config.js | 4 +- .../_config.js | 4 +- .../namespace-update-import-fails/_config.js | 4 +- .../function/reassign-import-fails/_config.js | 4 +- .../_config.js | 4 +- .../_config.js | 4 +- test/function/warn-on-eval/_config.js | 24 ++++++------ test/test.js | 12 +++++- 22 files changed, 125 insertions(+), 68 deletions(-) create mode 100644 src/utils/getCodeFrame.js delete mode 100644 src/utils/getLocation.js diff --git a/package.json b/package.json index f7af247..4911fac 100644 --- a/package.json +++ b/package.json @@ -53,6 +53,7 @@ "eslint-plugin-import": "^2.2.0", "is-reference": "^1.0.0", "istanbul": "^0.4.3", + "locate-character": "^2.0.0", "magic-string": "^0.15.2", "minimist": "^1.2.0", "mocha": "^3.0.0", diff --git a/src/Bundle.js b/src/Bundle.js index 1166cb2..9332b83 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -566,4 +566,9 @@ export default class Bundle { return ordered; } + + warn ( warning ) { + warning.toString = () => warning.message || warning; + this.onwarn( warning ); + } } diff --git a/src/Module.js b/src/Module.js index 71db738..97f6cca 100644 --- a/src/Module.js +++ b/src/Module.js @@ -1,10 +1,11 @@ -import { timeStart, timeEnd } from './utils/flushTime.js'; import { parse } from 'acorn/src/index.js'; import MagicString from 'magic-string'; +import { locate } from 'locate-character'; +import { timeStart, timeEnd } from './utils/flushTime.js'; import { assign, blank, deepClone, keys } from './utils/object.js'; import { basename, extname } from './utils/path.js'; -import getLocation from './utils/getLocation.js'; import makeLegalIdentifier from './utils/makeLegalIdentifier.js'; +import getCodeFrame from './utils/getCodeFrame.js'; import { SOURCEMAPPING_URL_RE } from './utils/sourceMappingURL.js'; import error from './utils/error.js'; import relativeId from './utils/relativeId.js'; @@ -195,7 +196,7 @@ export default class Module { if ( this.imports[ localName ] ) { const err = new Error( `Duplicated import '${localName}'` ); err.file = this.id; - err.loc = getLocation( this.code, specifier.start ); + err.loc = locate( this.code, specifier.start, { offsetLine: 1 }); throw err; } @@ -361,7 +362,7 @@ export default class Module { error({ message: `'${importDeclaration.name}' is not exported by ${relativeId( otherModule.id )} (imported by ${relativeId( this.id )}). For help fixing this error see https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module`, file: this.id, - loc: getLocation( this.code, importDeclaration.specifier.start ) + loc: locate( this.code, importDeclaration.specifier.start, { offsetLine: 1 }) }); } @@ -381,7 +382,7 @@ export default class Module { error({ message: `'${reexportDeclaration.localName}' is not exported by '${reexportDeclaration.module.id}' (imported by '${this.id}')`, file: this.id, - loc: getLocation( this.code, reexportDeclaration.start ) + loc: locate( this.code, reexportDeclaration.start, { offsetLine: 1 }) }); } @@ -405,4 +406,22 @@ export default class Module { if ( declaration ) return declaration; } } + + warn ( warning, pos ) { + if ( pos ) { + warning.pos = pos; + + const { line, column } = locate( this.code, pos, { offsetLine: 1 }); // TODO trace sourcemaps + + warning.loc = { + file: this.id, + line: line + 1, + column + }; + + warning.frame = getCodeFrame( this.code, line, column ); + } + + this.bundle.warn( warning ); + } } diff --git a/src/ast/Node.js b/src/ast/Node.js index 6a80ede..da36211 100644 --- a/src/ast/Node.js +++ b/src/ast/Node.js @@ -1,5 +1,5 @@ +import { locate } from 'locate-character'; import { UNKNOWN } from './values.js'; -import getLocation from '../utils/getLocation.js'; export default class Node { bind ( scope ) { @@ -74,7 +74,7 @@ export default class Node { locate () { // useful for debugging - const location = getLocation( this.module.code, this.start ); + const location = locate( this.module.code, this.start, { offsetLine: 1 }); location.file = this.module.id; location.toString = () => JSON.stringify( location ); diff --git a/src/ast/nodes/CallExpression.js b/src/ast/nodes/CallExpression.js index d275100..4880cb5 100644 --- a/src/ast/nodes/CallExpression.js +++ b/src/ast/nodes/CallExpression.js @@ -1,4 +1,4 @@ -import getLocation from '../../utils/getLocation.js'; +import { locate } from 'locate-character'; import error from '../../utils/error.js'; import Node from '../Node.js'; import isProgramLevel from '../utils/isProgramLevel.js'; @@ -14,12 +14,16 @@ export default class CallExpression extends Node { message: `Cannot call a namespace ('${this.callee.name}')`, file: this.module.id, pos: this.start, - loc: getLocation( this.module.code, this.start ) + loc: locate( this.module.code, this.start, { offsetLine: 1 }) }); } if ( this.callee.name === 'eval' && declaration.isGlobal ) { - this.module.bundle.onwarn( `Use of \`eval\` (in ${this.module.id}) is strongly discouraged, as it poses security risks and may cause issues with minification. See https://github.com/rollup/rollup/wiki/Troubleshooting#avoiding-eval for more details` ); + this.module.warn({ + code: 'EVAL', + message: `Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification`, + url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#avoiding-eval' + }, this.start ); } } diff --git a/src/ast/nodes/ExportDefaultDeclaration.js b/src/ast/nodes/ExportDefaultDeclaration.js index d70bd10..743287e 100644 --- a/src/ast/nodes/ExportDefaultDeclaration.js +++ b/src/ast/nodes/ExportDefaultDeclaration.js @@ -1,5 +1,5 @@ import Node from '../Node.js'; -import getLocation from '../../utils/getLocation.js'; +import { locate } from 'locate-character'; import relativeId from '../../utils/relativeId.js'; const functionOrClassDeclaration = /^(?:Function|Class)Declaration/; @@ -74,7 +74,7 @@ export default class ExportDefaultDeclaration extends Node { const newlineSeparated = /\n/.test( code.original.slice( start, end ) ); if ( newlineSeparated ) { - const { line, column } = getLocation( this.module.code, this.declaration.start ); + const { line, column } = locate( this.module.code, this.declaration.start, { offsetLine: 1 }); this.module.bundle.onwarn( `${relativeId( this.module.id )} (${line}:${column}) Ambiguous default export (is a call expression, but looks like a function declaration). See https://github.com/rollup/rollup/wiki/Troubleshooting#ambiguous-default-export` ); } } diff --git a/src/ast/nodes/MemberExpression.js b/src/ast/nodes/MemberExpression.js index bcdcf26..4da3031 100644 --- a/src/ast/nodes/MemberExpression.js +++ b/src/ast/nodes/MemberExpression.js @@ -1,5 +1,5 @@ import isReference from 'is-reference'; -import getLocation from '../../utils/getLocation.js'; +import { locate } from 'locate-character'; import relativeId from '../../utils/relativeId.js'; import Node from '../Node.js'; import { UNKNOWN } from '../values.js'; @@ -34,7 +34,7 @@ export default class MemberExpression extends Node { declaration = declaration.module.traceExport( part.name ); if ( !declaration ) { - const { line, column } = getLocation( this.module.code, this.start ); + const { line, column } = locate( this.module.code, this.start, { offsetLine: 1 }); this.module.bundle.onwarn( `${relativeId( this.module.id )} (${line}:${column}) '${part.name}' is not exported by '${relativeId( exporterId )}'. See https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` ); this.replacement = 'undefined'; return; diff --git a/src/ast/nodes/ThisExpression.js b/src/ast/nodes/ThisExpression.js index ae35173..666be58 100644 --- a/src/ast/nodes/ThisExpression.js +++ b/src/ast/nodes/ThisExpression.js @@ -1,5 +1,5 @@ import Node from '../Node.js'; -import getLocation from '../../utils/getLocation.js'; +import { locate } from 'locate-character'; import relativeId from '../../utils/relativeId.js'; const warning = `The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten. See https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined for more information`; @@ -11,7 +11,7 @@ export default class ThisExpression extends Node { if ( lexicalBoundary.isModuleScope ) { this.alias = this.module.context; if ( this.alias === 'undefined' ) { - const { line, column } = getLocation( this.module.code, this.start ); + const { line, column } = locate( this.module.code, this.start, { offsetLine: 1 }); const detail = `${relativeId( this.module.id )} (${line}:${column + 1})`; // use one-based column number convention this.module.bundle.onwarn( `${detail} ${warning}` ); } diff --git a/src/ast/nodes/shared/disallowIllegalReassignment.js b/src/ast/nodes/shared/disallowIllegalReassignment.js index a40969f..5d338af 100644 --- a/src/ast/nodes/shared/disallowIllegalReassignment.js +++ b/src/ast/nodes/shared/disallowIllegalReassignment.js @@ -1,4 +1,4 @@ -import getLocation from '../../../utils/getLocation.js'; +import { locate } from 'locate-character'; import error from '../../../utils/error.js'; // TODO tidy this up a bit (e.g. they can both use node.module.imports) @@ -10,7 +10,7 @@ export default function disallowIllegalReassignment ( scope, node ) { message: `Illegal reassignment to import '${node.object.name}'`, file: node.module.id, pos: node.start, - loc: getLocation( node.module.code, node.start ) + loc: locate( node.module.code, node.start, { offsetLine: 1 }) }); } } @@ -21,7 +21,7 @@ export default function disallowIllegalReassignment ( scope, node ) { message: `Illegal reassignment to import '${node.name}'`, file: node.module.id, pos: node.start, - loc: getLocation( node.module.code, node.start ) + loc: locate( node.module.code, node.start, { offsetLine: 1 }) }); } } diff --git a/src/utils/getCodeFrame.js b/src/utils/getCodeFrame.js new file mode 100644 index 0000000..369829b --- /dev/null +++ b/src/utils/getCodeFrame.js @@ -0,0 +1,38 @@ +function spaces ( i ) { + let result = ''; + while ( i-- ) result += ' '; + return result; +} + + +function tabsToSpaces ( str ) { + return str.replace( /^\t+/, match => match.split( '\t' ).join( ' ' ) ); +} + +export default function getCodeFrame ( source, line, column ) { + let lines = source.split( '\n' ); + + const frameStart = Math.max( 0, line - 3 ); + const frameEnd = Math.min( line + 2, lines.length ); + + const digits = String( frameEnd + 1 ).length; + + lines = lines.slice( frameStart, frameEnd ); + while ( !/\S/.test( lines[ lines.length - 1 ] ) ) lines.pop(); + + return lines + .map( ( str, i ) => { + const isErrorLine = frameStart + i + 1 === line; + + let lineNum = String( i + frameStart + 1 ); + while ( lineNum.length < digits ) lineNum = ` ${lineNum}`; + + if ( isErrorLine ) { + const indicator = spaces( digits + 2 + tabsToSpaces( str.slice( 0, column ) ).length ) + '^'; + return `${lineNum}: ${tabsToSpaces( str )}\n${indicator}`; + } + + return `${lineNum}: ${tabsToSpaces( str )}`; + }) + .join( '\n' ); +} diff --git a/src/utils/getLocation.js b/src/utils/getLocation.js deleted file mode 100644 index 41ca492..0000000 --- a/src/utils/getLocation.js +++ /dev/null @@ -1,20 +0,0 @@ -export default function getLocation ( source, charIndex ) { - const lines = source.split( '\n' ); - const len = lines.length; - - let lineStart = 0; - let i; - - for ( i = 0; i < len; i += 1 ) { - const line = lines[i]; - const lineEnd = lineStart + line.length + 1; // +1 for newline - - if ( lineEnd > charIndex ) { - return { line: i + 1, column: charIndex - lineStart }; - } - - lineStart = lineEnd; - } - - throw new Error( 'Could not determine location of character' ); -} diff --git a/test/function/cannot-call-external-namespace/_config.js b/test/function/cannot-call-external-namespace/_config.js index 263c14a..5ed02e2 100644 --- a/test/function/cannot-call-external-namespace/_config.js +++ b/test/function/cannot-call-external-namespace/_config.js @@ -7,6 +7,6 @@ module.exports = { assert.equal( err.message, 'Cannot call a namespace (\'foo\')' ); assert.equal( err.file.replace( /\//g, path.sep ), path.resolve( __dirname, 'main.js' ) ); assert.equal( err.pos, 28 ); - assert.deepEqual( err.loc, { line: 2, column: 0 }); + assert.deepEqual( err.loc, { character: 28, line: 2, column: 0 }); } }; diff --git a/test/function/cannot-call-internal-namespace/_config.js b/test/function/cannot-call-internal-namespace/_config.js index ceb42c3..d347008 100644 --- a/test/function/cannot-call-internal-namespace/_config.js +++ b/test/function/cannot-call-internal-namespace/_config.js @@ -7,6 +7,6 @@ module.exports = { assert.equal( err.message, 'Cannot call a namespace (\'foo\')' ); assert.equal( err.file.replace( /\//g, path.sep ), path.resolve( __dirname, 'main.js' ) ); assert.equal( err.pos, 33 ); - assert.deepEqual( err.loc, { line: 2, column: 0 }); + assert.deepEqual( err.loc, { character: 33, line: 2, column: 0 }); } }; diff --git a/test/function/duplicate-import-fails/_config.js b/test/function/duplicate-import-fails/_config.js index 4083690..492ac2f 100644 --- a/test/function/duplicate-import-fails/_config.js +++ b/test/function/duplicate-import-fails/_config.js @@ -4,8 +4,8 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows duplicate imports', error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 2, column: 9 }); + assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); + assert.deepEqual( err.loc, { character: 36, line: 2, column: 9 }); assert.ok( /Duplicated import/.test( err.message ) ); } }; diff --git a/test/function/duplicate-import-specifier-fails/_config.js b/test/function/duplicate-import-specifier-fails/_config.js index e3957b5..58446af 100644 --- a/test/function/duplicate-import-specifier-fails/_config.js +++ b/test/function/duplicate-import-specifier-fails/_config.js @@ -4,8 +4,8 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows duplicate import specifiers', error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 1, column: 12 }); + assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); + assert.deepEqual( err.loc, { character: 12, line: 1, column: 12 }); assert.ok( /Duplicated import/.test( err.message ) ); } }; diff --git a/test/function/namespace-reassign-import-fails/_config.js b/test/function/namespace-reassign-import-fails/_config.js index 2d606f4..a76a36a 100644 --- a/test/function/namespace-reassign-import-fails/_config.js +++ b/test/function/namespace-reassign-import-fails/_config.js @@ -4,8 +4,8 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows reassignments to namespace exports', error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 3, column: 0 }); + assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); + assert.deepEqual( err.loc, { character: 31, line: 3, column: 0 }); assert.ok( /Illegal reassignment/.test( err.message ) ); } }; diff --git a/test/function/namespace-update-import-fails/_config.js b/test/function/namespace-update-import-fails/_config.js index 4cff5c0..dc8e0f3 100644 --- a/test/function/namespace-update-import-fails/_config.js +++ b/test/function/namespace-update-import-fails/_config.js @@ -4,8 +4,8 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows updates to namespace exports', error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 3, column: 0 }); + assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); + assert.deepEqual( err.loc, { character: 31, line: 3, column: 0 }); assert.ok( /Illegal reassignment/.test( err.message ) ); } }; diff --git a/test/function/reassign-import-fails/_config.js b/test/function/reassign-import-fails/_config.js index 22591fe..4854d02 100644 --- a/test/function/reassign-import-fails/_config.js +++ b/test/function/reassign-import-fails/_config.js @@ -5,8 +5,8 @@ module.exports = { description: 'disallows assignments to imported bindings', error: function ( err ) { assert.ok( /Illegal reassignment/.test( err.message ) ); - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 8, column: 0 }); + assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); + assert.deepEqual( err.loc, { character: 113, line: 8, column: 0 }); } }; diff --git a/test/function/reassign-import-not-at-top-level-fails/_config.js b/test/function/reassign-import-not-at-top-level-fails/_config.js index 6e00786..b4cba92 100644 --- a/test/function/reassign-import-not-at-top-level-fails/_config.js +++ b/test/function/reassign-import-not-at-top-level-fails/_config.js @@ -4,8 +4,8 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows assignments to imported bindings not at the top level', error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 7, column: 2 }); + assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); + assert.deepEqual( err.loc, { character: 95, line: 7, column: 2 }); assert.ok( /Illegal reassignment/.test( err.message ) ); } }; diff --git a/test/function/update-expression-of-import-fails/_config.js b/test/function/update-expression-of-import-fails/_config.js index 5f5c27c..cf72241 100644 --- a/test/function/update-expression-of-import-fails/_config.js +++ b/test/function/update-expression-of-import-fails/_config.js @@ -4,8 +4,8 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows updates to imported bindings', error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 3, column: 0 }); + assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); + assert.deepEqual( err.loc, { character: 28, line: 3, column: 0 }); assert.ok( /Illegal reassignment/.test( err.message ) ); } }; diff --git a/test/function/warn-on-eval/_config.js b/test/function/warn-on-eval/_config.js index d6ac89f..7b499ef 100644 --- a/test/function/warn-on-eval/_config.js +++ b/test/function/warn-on-eval/_config.js @@ -1,16 +1,16 @@ -var assert = require( 'assert' ); - -var warned = false; - module.exports = { description: 'warns about use of eval', - options: { - onwarn: function ( message ) { - warned = true; - assert.ok( /Use of `eval` \(in .+?main\.js\) is strongly discouraged, as it poses security risks and may cause issues with minification\. See https:\/\/github.com\/rollup\/rollup\/wiki\/Troubleshooting#avoiding-eval for more details/.test( message ) ); + warnings: [ + { + code: 'EVAL', + message: `Use of eval is strongly discouraged, as it poses security risks and may cause issues with minification`, + pos: 13, + loc: { + column: 13, + file: require( 'path' ).resolve( __dirname, 'main.js' ), + line: 2 + }, + url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#avoiding-eval' } - }, - exports: function () { - assert.ok( warned, 'did not warn' ); - } + ] }; diff --git a/test/test.js b/test/test.js index 50f63e8..8b1d3cf 100644 --- a/test/test.js +++ b/test/test.js @@ -270,7 +270,17 @@ describe( 'rollup', function () { } if ( config.warnings ) { - config.warnings( warnings ); + if ( Array.isArray( config.warnings ) ) { + assert.deepEqual( warnings.map( warning => { + const clone = Object.assign( {}, warning ); + delete clone.toString; + delete clone.frame; + + return clone; + }), config.warnings ); + } else { + config.warnings( warnings ); + } } else if ( warnings.length ) { throw new Error( `Got unexpected warnings:\n${warnings.join('\n')}` ); } From 4dba546885d927988b55efcdb85939278cfa92e4 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 15:56:42 -0500 Subject: [PATCH 05/28] use bundle.warn and module.warn throughout codebase --- src/Bundle.js | 30 ++++++++-- src/Module.js | 16 ++--- src/ast/nodes/ExportDefaultDeclaration.js | 7 ++- src/ast/nodes/MemberExpression.js | 8 ++- src/ast/nodes/ThisExpression.js | 12 ++-- src/ast/scopes/ModuleScope.js | 6 +- src/finalisers/iife.js | 2 +- src/finalisers/shared/getGlobalNameMaker.js | 8 ++- src/finalisers/shared/warnOnBuiltins.js | 5 +- src/finalisers/umd.js | 2 +- src/utils/collapseSourcemaps.js | 8 ++- src/utils/getExportMode.js | 6 +- .../assign-namespace-to-var/_config.js | 11 ++-- .../custom-path-resolver-async/_config.js | 12 ++-- .../custom-path-resolver-sync/_config.js | 12 ++-- .../_config.js | 12 ++-- .../double-named-export-from/_config.js | 13 ---- test/function/empty-exports/_config.js | 29 ++++++--- test/function/module-tree/_config.js | 11 ++-- .../namespace-missing-export/_config.js | 24 ++++++-- test/function/unused-import/_config.js | 22 ++++--- .../_config.js | 24 ++++++-- .../_config.js | 14 ++--- test/function/warn-on-empty-bundle/_config.js | 13 ++-- test/function/warn-on-eval/_config.js | 6 +- .../warn-on-namespace-conflict/_config.js | 9 +++ .../bar.js | 0 .../deep.js | 0 .../foo.js | 0 .../main.js | 0 .../warn-on-top-level-this/_config.js | 24 ++++++-- .../warn-on-unused-missing-imports/_config.js | 23 ++++++-- .../transform-without-sourcemap/_config.js | 22 +++---- test/test.js | 59 ++++++++++++++----- 34 files changed, 295 insertions(+), 155 deletions(-) delete mode 100644 test/function/double-named-export-from/_config.js create mode 100644 test/function/warn-on-namespace-conflict/_config.js rename test/function/{double-named-export-from => warn-on-namespace-conflict}/bar.js (100%) rename test/function/{double-named-export-from => warn-on-namespace-conflict}/deep.js (100%) rename test/function/{double-named-export-from => warn-on-namespace-conflict}/foo.js (100%) rename test/function/{double-named-export-from => warn-on-namespace-conflict}/main.js (100%) diff --git a/src/Bundle.js b/src/Bundle.js index 9332b83..f2f0dd2 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -188,7 +188,10 @@ export default class Bundle { `'${unused[0]}' is` : `${unused.slice( 0, -1 ).map( name => `'${name}'` ).join( ', ' )} and '${unused.pop()}' are`; - this.onwarn( `${names} imported from external module '${module.id}' but never used` ); + this.warn({ + code: 'UNUSED_EXTERNAL_IMPORT', + message: `${names} imported from external module '${module.id}' but never used` + }); }); this.orderedModules = this.sort(); @@ -309,7 +312,10 @@ export default class Bundle { keys( exportAllModule.exportsAll ).forEach( name => { if ( name in module.exportsAll ) { - this.onwarn( `Conflicting namespaces: ${module.id} re-exports '${name}' from both ${module.exportsAll[ name ]} (will be ignored) and ${exportAllModule.exportsAll[ name ]}.` ); + this.warn({ + code: 'NAMESPACE_CONFLICT', + message: `Conflicting namespaces: ${relativeId( module.id )} re-exports '${name}' from both ${relativeId( module.exportsAll[ name ] )} (will be ignored) and ${relativeId( exportAllModule.exportsAll[ name ] )}` + }); } module.exportsAll[ name ] = exportAllModule.exportsAll[ name ]; }); @@ -333,7 +339,11 @@ export default class Bundle { if ( !resolvedId && !isExternal ) { if ( isRelative( source ) ) throw new Error( `Could not resolve '${source}' from ${module.id}` ); - this.onwarn( `'${source}' is imported by ${relativeId( module.id )}, but could not be resolved – treating it as an external dependency. For help see https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` ); + this.warn({ + code: 'UNRESOLVED_IMPORT', + message: `'${source}' is imported by ${relativeId( module.id )}, but could not be resolved – treating it as an external dependency`, + url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency' + }); isExternal = true; } @@ -380,7 +390,11 @@ export default class Bundle { render ( options = {} ) { if ( options.format === 'es6' ) { - this.onwarn( 'The es6 format is deprecated – use `es` instead' ); + this.warn({ + code: 'DEPRECATED_ES6', + message: 'The es6 format is deprecated – use `es` instead' + }); + options.format = 'es'; } @@ -404,7 +418,10 @@ export default class Bundle { }); if ( !magicString.toString().trim() && this.entryModule.getExports().length === 0 ) { - this.onwarn( 'Generated an empty bundle' ); + this.warn({ + code: 'EMPTY_BUNDLE', + message: 'Generated an empty bundle' + }); } timeEnd( 'render modules' ); @@ -470,7 +487,7 @@ export default class Bundle { if ( typeof map.mappings === 'string' ) { map.mappings = decode( map.mappings ); } - map = collapseSourcemaps( file, map, usedModules, bundleSourcemapChain, this.onwarn ); + map = collapseSourcemaps( this, file, map, usedModules, bundleSourcemapChain ); } else { map = magicString.generateMap({ file, includeContent: true }); } @@ -535,6 +552,7 @@ export default class Bundle { for ( i += 1; i < ordered.length; i += 1 ) { const b = ordered[i]; + // TODO reinstate this! it no longer works if ( stronglyDependsOn[ a.id ][ b.id ] ) { // somewhere, there is a module that imports b before a. Because // b imports a, a is placed before b. We need to find the module diff --git a/src/Module.js b/src/Module.js index 97f6cca..81926c3 100644 --- a/src/Module.js +++ b/src/Module.js @@ -180,7 +180,12 @@ export default class Module { this.exports[ exportedName ] = { localName }; }); } else { - this.bundle.onwarn( `Module ${this.id} has an empty export declaration` ); + // TODO is this really necessary? `export {}` is valid JS, and + // might be used as a hint that this is indeed a module + this.warn({ + code: 'EMPTY_EXPORT', + message: `Empty export declaration` + }, node.start ); } } } @@ -408,17 +413,12 @@ export default class Module { } warn ( warning, pos ) { - if ( pos ) { + if ( pos !== undefined ) { warning.pos = pos; const { line, column } = locate( this.code, pos, { offsetLine: 1 }); // TODO trace sourcemaps - warning.loc = { - file: this.id, - line: line + 1, - column - }; - + warning.loc = { file: this.id, line, column }; warning.frame = getCodeFrame( this.code, line, column ); } diff --git a/src/ast/nodes/ExportDefaultDeclaration.js b/src/ast/nodes/ExportDefaultDeclaration.js index 743287e..208120f 100644 --- a/src/ast/nodes/ExportDefaultDeclaration.js +++ b/src/ast/nodes/ExportDefaultDeclaration.js @@ -74,8 +74,11 @@ export default class ExportDefaultDeclaration extends Node { const newlineSeparated = /\n/.test( code.original.slice( start, end ) ); if ( newlineSeparated ) { - const { line, column } = locate( this.module.code, this.declaration.start, { offsetLine: 1 }); - this.module.bundle.onwarn( `${relativeId( this.module.id )} (${line}:${column}) Ambiguous default export (is a call expression, but looks like a function declaration). See https://github.com/rollup/rollup/wiki/Troubleshooting#ambiguous-default-export` ); + this.module.warn({ + code: 'AMBIGUOUS_DEFAULT_EXPORT', + message: `Ambiguous default export (is a call expression, but looks like a function declaration)`, + url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#ambiguous-default-export' + }, this.declaration.start ); } } } diff --git a/src/ast/nodes/MemberExpression.js b/src/ast/nodes/MemberExpression.js index 4da3031..d745d7d 100644 --- a/src/ast/nodes/MemberExpression.js +++ b/src/ast/nodes/MemberExpression.js @@ -1,5 +1,4 @@ import isReference from 'is-reference'; -import { locate } from 'locate-character'; import relativeId from '../../utils/relativeId.js'; import Node from '../Node.js'; import { UNKNOWN } from '../values.js'; @@ -34,8 +33,11 @@ export default class MemberExpression extends Node { declaration = declaration.module.traceExport( part.name ); if ( !declaration ) { - const { line, column } = locate( this.module.code, this.start, { offsetLine: 1 }); - this.module.bundle.onwarn( `${relativeId( this.module.id )} (${line}:${column}) '${part.name}' is not exported by '${relativeId( exporterId )}'. See https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` ); + this.module.warn({ + code: 'MISSING_EXPORT', + message: `'${part.name}' is not exported by '${relativeId( exporterId )}'`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` + }, part.start ); this.replacement = 'undefined'; return; } diff --git a/src/ast/nodes/ThisExpression.js b/src/ast/nodes/ThisExpression.js index 666be58..2429c65 100644 --- a/src/ast/nodes/ThisExpression.js +++ b/src/ast/nodes/ThisExpression.js @@ -1,8 +1,4 @@ import Node from '../Node.js'; -import { locate } from 'locate-character'; -import relativeId from '../../utils/relativeId.js'; - -const warning = `The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten. See https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined for more information`; export default class ThisExpression extends Node { initialise ( scope ) { @@ -11,9 +7,11 @@ export default class ThisExpression extends Node { if ( lexicalBoundary.isModuleScope ) { this.alias = this.module.context; if ( this.alias === 'undefined' ) { - const { line, column } = locate( this.module.code, this.start, { offsetLine: 1 }); - const detail = `${relativeId( this.module.id )} (${line}:${column + 1})`; // use one-based column number convention - this.module.bundle.onwarn( `${detail} ${warning}` ); + this.module.warn({ + code: 'THIS_IS_UNDEFINED', + message: `The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined` + }, this.start ); } } } diff --git a/src/ast/scopes/ModuleScope.js b/src/ast/scopes/ModuleScope.js index 4c8804c..f0f5f7a 100644 --- a/src/ast/scopes/ModuleScope.js +++ b/src/ast/scopes/ModuleScope.js @@ -1,4 +1,5 @@ import { forOwn } from '../../utils/object.js'; +import relativeId from '../../utils/relativeId.js'; import Scope from './Scope.js'; export default class ModuleScope extends Scope { @@ -26,7 +27,10 @@ export default class ModuleScope extends Scope { if ( specifier.name !== '*' ) { const declaration = specifier.module.traceExport( specifier.name ); if ( !declaration ) { - this.module.bundle.onwarn( `Non-existent export '${specifier.name}' is imported from ${specifier.module.id} by ${this.module.id}` ); + this.module.warn({ + code: 'NON_EXISTENT_EXPORT', + message: `Non-existent export '${specifier.name}' is imported from ${relativeId( specifier.module.id )}` + }, specifier.specifier.start ); return; } diff --git a/src/finalisers/iife.js b/src/finalisers/iife.js index f6d702e..2d6121e 100644 --- a/src/finalisers/iife.js +++ b/src/finalisers/iife.js @@ -25,7 +25,7 @@ function setupNamespace ( keypath ) { } export default function iife ( bundle, magicString, { exportMode, indentString, intro, outro }, options ) { - const globalNameMaker = getGlobalNameMaker( options.globals || blank(), bundle.onwarn ); + const globalNameMaker = getGlobalNameMaker( options.globals || blank(), bundle ); const name = options.moduleName; const isNamespaced = name && ~name.indexOf( '.' ); diff --git a/src/finalisers/shared/getGlobalNameMaker.js b/src/finalisers/shared/getGlobalNameMaker.js index 676b416..7a1d622 100644 --- a/src/finalisers/shared/getGlobalNameMaker.js +++ b/src/finalisers/shared/getGlobalNameMaker.js @@ -1,11 +1,15 @@ -export default function getGlobalNameMaker ( globals, onwarn ) { +export default function getGlobalNameMaker ( globals, bundle ) { const fn = typeof globals === 'function' ? globals : id => globals[ id ]; return function ( module ) { const name = fn( module.id ); if ( name ) return name; - onwarn( `No name was provided for external module '${module.id}' in options.globals – guessing '${module.name}'` ); + bundle.warn({ + code: 'MISSING_GLOBAL_NAME', + message: `No name was provided for external module '${module.id}' in options.globals – guessing '${module.name}'` + }); + return module.name; }; } diff --git a/src/finalisers/shared/warnOnBuiltins.js b/src/finalisers/shared/warnOnBuiltins.js index e5ac1ba..c4d63f6 100644 --- a/src/finalisers/shared/warnOnBuiltins.js +++ b/src/finalisers/shared/warnOnBuiltins.js @@ -35,5 +35,8 @@ export default function warnOnBuiltins ( bundle ) { `module ('${externalBuiltins[0]}')` : `modules (${externalBuiltins.slice( 0, -1 ).map( name => `'${name}'` ).join( ', ' )} and '${externalBuiltins.pop()}')`; - bundle.onwarn( `Creating a browser bundle that depends on Node.js built-in ${detail}. You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins` ); + bundle.warn({ + code: 'MISSING_NODE_BUILTINS', + message: `Creating a browser bundle that depends on Node.js built-in ${detail}. You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins` + }); } diff --git a/src/finalisers/umd.js b/src/finalisers/umd.js index c176242..655c77a 100644 --- a/src/finalisers/umd.js +++ b/src/finalisers/umd.js @@ -33,7 +33,7 @@ export default function umd ( bundle, magicString, { exportMode, indentString, i warnOnBuiltins( bundle ); - const globalNameMaker = getGlobalNameMaker( options.globals || blank(), bundle.onwarn ); + const globalNameMaker = getGlobalNameMaker( options.globals || blank(), bundle ); const amdDeps = bundle.externalModules.map( quotePath ); const cjsDeps = bundle.externalModules.map( req ); diff --git a/src/utils/collapseSourcemaps.js b/src/utils/collapseSourcemaps.js index 7a504ba..122a148 100644 --- a/src/utils/collapseSourcemaps.js +++ b/src/utils/collapseSourcemaps.js @@ -98,7 +98,7 @@ class Link { } } -export default function collapseSourcemaps ( file, map, modules, bundleSourcemapChain, onwarn ) { +export default function collapseSourcemaps ( bundle, file, map, modules, bundleSourcemapChain ) { const moduleSources = modules.filter( module => !module.excludeFromSourcemap ).map( module => { let sourceMapChain = module.sourceMapChain; @@ -127,7 +127,11 @@ export default function collapseSourcemaps ( file, map, modules, bundleSourcemap sourceMapChain.forEach( map => { if ( map.missing ) { - onwarn( `Sourcemap is likely to be incorrect: a plugin${map.plugin ? ` ('${map.plugin}')` : ``} was used to transform files, but didn't generate a sourcemap for the transformation. Consult https://github.com/rollup/rollup/wiki/Troubleshooting and the plugin documentation for more information` ); + bundle.warn({ + code: 'SOURCEMAP_BROKEN', + message: `Sourcemap is likely to be incorrect: a plugin${map.plugin ? ` ('${map.plugin}')` : ``} was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#sourcemap-is-likely-to-be-incorrect` + }); map = { names: [], diff --git a/src/utils/getExportMode.js b/src/utils/getExportMode.js index a3d00a0..d525550 100644 --- a/src/utils/getExportMode.js +++ b/src/utils/getExportMode.js @@ -24,7 +24,11 @@ export default function getExportMode ( bundle, {exports: exportMode, moduleName exportMode = 'default'; } else { if ( bundle.entryModule.exports.default && format !== 'es') { - bundle.onwarn( `Using named and default exports together. Consumers of your bundle will have to use ${moduleName || 'bundle'}['default'] to access the default export, which may not be what you want. Use \`exports: 'named'\` to disable this warning. See https://github.com/rollup/rollup/wiki/JavaScript-API#exports for more information` ); + bundle.warn({ + code: 'MIXED_EXPORTS', + message: `Using named and default exports together. Consumers of your bundle will have to use ${moduleName || 'bundle'}['default'] to access the default export, which may not be what you want. Use \`exports: 'named'\` to disable this warning`, + url: `https://github.com/rollup/rollup/wiki/JavaScript-API#exports` + }); } exportMode = 'named'; } diff --git a/test/function/assign-namespace-to-var/_config.js b/test/function/assign-namespace-to-var/_config.js index 2eb7e9c..a8c94ea 100644 --- a/test/function/assign-namespace-to-var/_config.js +++ b/test/function/assign-namespace-to-var/_config.js @@ -2,9 +2,10 @@ const assert = require( 'assert' ); module.exports = { description: 'allows a namespace to be assigned to a variable', - warnings: warnings => { - assert.deepEqual( warnings, [ - 'Generated an empty bundle' - ]); - } + warnings: [ + { + code: 'EMPTY_BUNDLE', + message: 'Generated an empty bundle' + } + ] }; diff --git a/test/function/custom-path-resolver-async/_config.js b/test/function/custom-path-resolver-async/_config.js index 83a10c1..3924db7 100644 --- a/test/function/custom-path-resolver-async/_config.js +++ b/test/function/custom-path-resolver-async/_config.js @@ -21,11 +21,13 @@ module.exports = { } }] }, - warnings: function ( warnings ) { - assert.deepEqual( warnings, [ - `'path' is imported by main.js, but could not be resolved – treating it as an external dependency. For help see https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` - ]); - }, + warnings: [ + { + code: 'UNRESOLVED_IMPORT', + message: `'path' is imported by main.js, but could not be resolved – treating it as an external dependency`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` + } + ], exports: function ( exports ) { assert.strictEqual( exports.path, require( 'path' ) ); } diff --git a/test/function/custom-path-resolver-sync/_config.js b/test/function/custom-path-resolver-sync/_config.js index 074f671..f32cb9a 100644 --- a/test/function/custom-path-resolver-sync/_config.js +++ b/test/function/custom-path-resolver-sync/_config.js @@ -13,11 +13,13 @@ module.exports = { } }] }, - warnings: function ( warnings ) { - assert.deepEqual( warnings, [ - `'path' is imported by main.js, but could not be resolved – treating it as an external dependency. For help see https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` - ]); - }, + warnings: [ + { + code: 'UNRESOLVED_IMPORT', + message: `'path' is imported by main.js, but could not be resolved – treating it as an external dependency`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` + } + ], exports: function ( exports ) { assert.strictEqual( exports.path, require( 'path' ) ); } diff --git a/test/function/does-not-hang-on-missing-module/_config.js b/test/function/does-not-hang-on-missing-module/_config.js index 901bfae..aeb5386 100644 --- a/test/function/does-not-hang-on-missing-module/_config.js +++ b/test/function/does-not-hang-on-missing-module/_config.js @@ -2,11 +2,13 @@ var assert = require( 'assert' ); module.exports = { description: 'does not hang on missing module (#53)', - warnings: warnings => { - assert.deepEqual( warnings, [ - `'unlessYouCreatedThisFileForSomeReason' is imported by main.js, but could not be resolved – treating it as an external dependency. For help see https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` - ]); - }, + warnings: [ + { + code: 'UNRESOLVED_IMPORT', + message: `'unlessYouCreatedThisFileForSomeReason' is imported by main.js, but could not be resolved – treating it as an external dependency`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` + } + ], runtimeError: function ( error ) { assert.equal( "Cannot find module 'unlessYouCreatedThisFileForSomeReason'", error.message ); } diff --git a/test/function/double-named-export-from/_config.js b/test/function/double-named-export-from/_config.js deleted file mode 100644 index b3885d2..0000000 --- a/test/function/double-named-export-from/_config.js +++ /dev/null @@ -1,13 +0,0 @@ -const { resolve } = require('path'); -const assert = require( 'assert' ); - -const r = path => resolve( __dirname, path ); - -module.exports = { - description: 'throws on duplicate export * from', - warnings ( warnings ) { - assert.deepEqual( warnings, [ - `Conflicting namespaces: ${r('main.js')} re-exports 'foo' from both ${r('foo.js')} (will be ignored) and ${r('deep.js')}.` - ]); - } -}; diff --git a/test/function/empty-exports/_config.js b/test/function/empty-exports/_config.js index 8030197..e928a59 100644 --- a/test/function/empty-exports/_config.js +++ b/test/function/empty-exports/_config.js @@ -1,12 +1,23 @@ -const assert = require( 'assert' ); -const path = require( 'path' ); - module.exports = { description: 'warns on export {}, but does not fail', - warnings: warnings => { - assert.deepEqual( warnings, [ - `Module ${path.resolve( __dirname, 'main.js' )} has an empty export declaration`, - 'Generated an empty bundle' - ]); - } + warnings: [ + { + code: 'EMPTY_EXPORT', + message: 'Empty export declaration', + pos: 0, + loc: { + file: require( 'path' ).resolve( __dirname, 'main.js' ), + line: 1, + column: 0 + }, + frame: ` + 1: export {}; + ^ + ` + }, + { + code: 'EMPTY_BUNDLE', + message: 'Generated an empty bundle' + } + ] }; diff --git a/test/function/module-tree/_config.js b/test/function/module-tree/_config.js index 9662844..601ee7d 100644 --- a/test/function/module-tree/_config.js +++ b/test/function/module-tree/_config.js @@ -34,9 +34,10 @@ module.exports = { } ]); }, - warnings: warnings => { - assert.deepEqual( warnings, [ - 'Generated an empty bundle' - ]); - } + warnings: [ + { + code: 'EMPTY_BUNDLE', + message: 'Generated an empty bundle' + } + ] }; diff --git a/test/function/namespace-missing-export/_config.js b/test/function/namespace-missing-export/_config.js index 6ecf3b9..485a1ba 100644 --- a/test/function/namespace-missing-export/_config.js +++ b/test/function/namespace-missing-export/_config.js @@ -1,9 +1,21 @@ -var assert = require( 'assert' ); - module.exports = { - options: { - onwarn: function ( msg ) { - assert.equal( msg, `main.js (3:21) 'foo' is not exported by 'empty.js'. See https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` ); + warnings: [ + { + code: 'MISSING_EXPORT', + message: `'foo' is not exported by 'empty.js'`, + pos: 61, + loc: { + file: require( 'path' ).resolve( __dirname, 'main.js' ), + line: 3, + column: 25 + }, + frame: ` + 1: import * as mod from './empty.js'; + 2: + 3: assert.equal( typeof mod.foo, 'undefined' ); + ^ + `, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` } - } + ] }; diff --git a/test/function/unused-import/_config.js b/test/function/unused-import/_config.js index b9e9b85..0f99f59 100644 --- a/test/function/unused-import/_config.js +++ b/test/function/unused-import/_config.js @@ -2,11 +2,19 @@ const assert = require( 'assert' ); module.exports = { description: 'warns on unused imports ([#595])', - warnings: warnings => { - assert.deepEqual( warnings, [ - `'external' is imported by main.js, but could not be resolved – treating it as an external dependency. For help see https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency`, - `'unused', 'notused' and 'neverused' are imported from external module 'external' but never used`, - `Generated an empty bundle` - ]); - } + warnings: [ + { + code: 'UNRESOLVED_IMPORT', + message: `'external' is imported by main.js, but could not be resolved – treating it as an external dependency`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency` + }, + { + code: 'UNUSED_EXTERNAL_IMPORT', + message: `'unused', 'notused' and 'neverused' are imported from external module 'external' but never used` + }, + { + code: 'EMPTY_BUNDLE', + message: `Generated an empty bundle` + } + ] }; diff --git a/test/function/warn-on-ambiguous-function-export/_config.js b/test/function/warn-on-ambiguous-function-export/_config.js index 1b83422..baa08bc 100644 --- a/test/function/warn-on-ambiguous-function-export/_config.js +++ b/test/function/warn-on-ambiguous-function-export/_config.js @@ -2,9 +2,23 @@ const assert = require( 'assert' ); module.exports = { description: 'uses original name of default export function (#1011)', - warnings: warnings => { - assert.deepEqual( warnings, [ - 'foo.js (1:15) Ambiguous default export (is a call expression, but looks like a function declaration). See https://github.com/rollup/rollup/wiki/Troubleshooting#ambiguous-default-export' - ]); - } + warnings: [ + { + code: 'AMBIGUOUS_DEFAULT_EXPORT', + message: `Ambiguous default export (is a call expression, but looks like a function declaration)`, + pos: 15, + loc: { + file: require( 'path' ).resolve( __dirname, 'foo.js' ), + line: 1, + column: 15 + }, + frame: ` + 1: export default function foo ( a, b ) { + ^ + 2: assert.equal( a, b ); + 3: return 3; + `, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#ambiguous-default-export` + } + ] }; diff --git a/test/function/warn-on-auto-named-default-exports/_config.js b/test/function/warn-on-auto-named-default-exports/_config.js index f093781..44e493b 100644 --- a/test/function/warn-on-auto-named-default-exports/_config.js +++ b/test/function/warn-on-auto-named-default-exports/_config.js @@ -1,10 +1,10 @@ -var assert = require( 'assert' ); - module.exports = { description: 'warns if default and named exports are used in auto mode', - warnings: function ( warnings ) { - assert.deepEqual( warnings, [ - 'Using named and default exports together. Consumers of your bundle will have to use bundle[\'default\'] to access the default export, which may not be what you want. Use `exports: \'named\'` to disable this warning. See https://github.com/rollup/rollup/wiki/JavaScript-API#exports for more information' - ]); - } + warnings: [ + { + code: 'MIXED_EXPORTS', + message: `Using named and default exports together. Consumers of your bundle will have to use bundle['default'] to access the default export, which may not be what you want. Use \`exports: 'named'\` to disable this warning`, + url: `https://github.com/rollup/rollup/wiki/JavaScript-API#exports` + } + ] }; diff --git a/test/function/warn-on-empty-bundle/_config.js b/test/function/warn-on-empty-bundle/_config.js index 2e599ad..a6d0a23 100644 --- a/test/function/warn-on-empty-bundle/_config.js +++ b/test/function/warn-on-empty-bundle/_config.js @@ -1,10 +1,9 @@ -const assert = require( 'assert' ); - module.exports = { description: 'warns if empty bundle is generated (#444)', - warnings: warnings => { - assert.deepEqual( warnings, [ - 'Generated an empty bundle' - ]); - } + warnings: [ + { + code: 'EMPTY_BUNDLE', + message: 'Generated an empty bundle' + } + ] }; diff --git a/test/function/warn-on-eval/_config.js b/test/function/warn-on-eval/_config.js index 7b499ef..a959bce 100644 --- a/test/function/warn-on-eval/_config.js +++ b/test/function/warn-on-eval/_config.js @@ -8,8 +8,12 @@ module.exports = { loc: { column: 13, file: require( 'path' ).resolve( __dirname, 'main.js' ), - line: 2 + line: 1 }, + frame: ` + 1: var result = eval( '1 + 1' ); + ^ + `, url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#avoiding-eval' } ] diff --git a/test/function/warn-on-namespace-conflict/_config.js b/test/function/warn-on-namespace-conflict/_config.js new file mode 100644 index 0000000..2054d75 --- /dev/null +++ b/test/function/warn-on-namespace-conflict/_config.js @@ -0,0 +1,9 @@ +module.exports = { + description: 'warns on duplicate export * from', + warnings: [ + { + code: 'NAMESPACE_CONFLICT', + message: `Conflicting namespaces: main.js re-exports 'foo' from both foo.js (will be ignored) and deep.js` + } + ] +}; diff --git a/test/function/double-named-export-from/bar.js b/test/function/warn-on-namespace-conflict/bar.js similarity index 100% rename from test/function/double-named-export-from/bar.js rename to test/function/warn-on-namespace-conflict/bar.js diff --git a/test/function/double-named-export-from/deep.js b/test/function/warn-on-namespace-conflict/deep.js similarity index 100% rename from test/function/double-named-export-from/deep.js rename to test/function/warn-on-namespace-conflict/deep.js diff --git a/test/function/double-named-export-from/foo.js b/test/function/warn-on-namespace-conflict/foo.js similarity index 100% rename from test/function/double-named-export-from/foo.js rename to test/function/warn-on-namespace-conflict/foo.js diff --git a/test/function/double-named-export-from/main.js b/test/function/warn-on-namespace-conflict/main.js similarity index 100% rename from test/function/double-named-export-from/main.js rename to test/function/warn-on-namespace-conflict/main.js diff --git a/test/function/warn-on-top-level-this/_config.js b/test/function/warn-on-top-level-this/_config.js index 99c3f2c..4d9033e 100644 --- a/test/function/warn-on-top-level-this/_config.js +++ b/test/function/warn-on-top-level-this/_config.js @@ -2,11 +2,25 @@ const assert = require( 'assert' ); module.exports = { description: 'warns on top-level this (#770)', - warnings: warnings => { - assert.deepEqual( warnings, [ - `main.js (3:1) The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten. See https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined for more information` - ]); - }, + warnings: [ + { + code: 'THIS_IS_UNDEFINED', + message: `The 'this' keyword is equivalent to 'undefined' at the top level of an ES module, and has been rewritten`, + pos: 81, + loc: { + file: require( 'path' ).resolve( __dirname, 'main.js' ), + line: 3, + column: 0 + }, + frame: ` + 1: const someVariableJustToCheckOnCorrectLineNumber = true; // eslint-disable-line + 2: + 3: this.foo = 'bar'; + ^ + `, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#this-is-undefined` + } + ], runtimeError: err => { assert.equal( err.message, `Cannot set property 'foo' of undefined` ); } diff --git a/test/function/warn-on-unused-missing-imports/_config.js b/test/function/warn-on-unused-missing-imports/_config.js index 1fca512..08cc6ba 100644 --- a/test/function/warn-on-unused-missing-imports/_config.js +++ b/test/function/warn-on-unused-missing-imports/_config.js @@ -3,9 +3,22 @@ const assert = require( 'assert' ); module.exports = { description: 'warns on missing (but unused) imports', - warnings: warnings => { - assert.deepEqual( warnings, [ - `Non-existent export 'b' is imported from ${path.resolve(__dirname, 'foo.js')} by ${path.resolve(__dirname, 'main.js')}` - ]); - } + warnings: [ + { + code: 'NON_EXISTENT_EXPORT', + message: `Non-existent export 'b' is imported from foo.js`, + pos: 12, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 1, + column: 12 + }, + frame: ` + 1: import { a, b } from './foo.js'; + ^ + 2: + 3: assert.equal( a, 42 ); + ` + } + ] }; diff --git a/test/sourcemaps/transform-without-sourcemap/_config.js b/test/sourcemaps/transform-without-sourcemap/_config.js index ad33354..0ce79c0 100644 --- a/test/sourcemaps/transform-without-sourcemap/_config.js +++ b/test/sourcemaps/transform-without-sourcemap/_config.js @@ -1,10 +1,5 @@ -const assert = require( 'assert' ); - -let warnings = []; - module.exports = { description: 'preserves sourcemap chains when transforming', - before: () => warnings = [], // reset options: { plugins: [ { @@ -13,14 +8,13 @@ module.exports = { return code; } } - ], - onwarn ( msg ) { - warnings.push( msg ); - } + ] }, - test: () => { - assert.deepEqual( warnings, [ - `Sourcemap is likely to be incorrect: a plugin ('fake plugin') was used to transform files, but didn't generate a sourcemap for the transformation. Consult https://github.com/rollup/rollup/wiki/Troubleshooting and the plugin documentation for more information` - ]); - } + warnings: [ + { + code: `SOURCEMAP_BROKEN`, + message: `Sourcemap is likely to be incorrect: a plugin ('fake plugin') was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#sourcemap-is-likely-to-be-incorrect` + } + ] }; diff --git a/test/test.js b/test/test.js index 8b1d3cf..789ac44 100644 --- a/test/test.js +++ b/test/test.js @@ -58,6 +58,27 @@ function loader ( modules ) { }; } +function compareWarnings ( actual, expected ) { + assert.deepEqual( + actual.map( warning => { + const clone = Object.assign( {}, warning ); + delete clone.toString; + + if ( clone.frame ) { + clone.frame = clone.frame.replace( /\s+$/gm, '' ); + } + + return clone; + }), + expected.map( warning => { + if ( warning.frame ) { + warning.frame = warning.frame.slice( 1 ).replace( /^\t+/gm, '' ).replace( /\s+$/gm, '' ).trim(); + } + return warning; + }) + ); +} + describe( 'rollup', function () { this.timeout( 10000 ); @@ -271,13 +292,7 @@ describe( 'rollup', function () { if ( config.warnings ) { if ( Array.isArray( config.warnings ) ) { - assert.deepEqual( warnings.map( warning => { - const clone = Object.assign( {}, warning ); - delete clone.toString; - delete clone.frame; - - return clone; - }), config.warnings ); + compareWarnings( warnings, config.warnings ); } else { config.warnings( warnings ); } @@ -387,11 +402,18 @@ describe( 'rollup', function () { const entry = path.resolve( SOURCEMAPS, dir, 'main.js' ); const dest = path.resolve( SOURCEMAPS, dir, '_actual/bundle' ); - const options = extend( {}, config.options, { entry }); + let warnings; + + const options = extend( {}, config.options, { + entry, + onwarn: warning => warnings.push( warning ) + }); PROFILES.forEach( profile => { ( config.skip ? it.skip : config.solo ? it.only : it )( 'generates ' + profile.format, () => { process.chdir( SOURCEMAPS + '/' + dir ); + warnings = []; + return rollup.rollup( options ).then( bundle => { const options = extend( {}, { format: profile.format, @@ -401,9 +423,16 @@ describe( 'rollup', function () { bundle.write( options ); - if ( config.before ) config.before(); - const result = bundle.generate( options ); - config.test( result.code, result.map ); + if ( config.test ) { + const { code, map } = bundle.generate( options ); + config.test( code, map ); + } + + if ( config.warnings ) { + compareWarnings( warnings, config.warnings ); + } else if ( warnings.length ) { + throw new Error( `Unexpected warnings` ); + } }); }); }); @@ -747,11 +776,9 @@ describe( 'rollup', function () { moduleName: 'myBundle' }); - assert.deepEqual( warnings, [ - `'util' is imported by entry, but could not be resolved – treating it as an external dependency. For help see https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency`, - `Creating a browser bundle that depends on Node.js built-in module ('util'). You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins`, - `No name was provided for external module 'util' in options.globals – guessing 'util'` - ]); + const relevantWarnings = warnings.filter( warning => warning.code === 'MISSING_NODE_BUILTINS' ); + assert.equal( relevantWarnings.length, 1 ); + assert.equal( relevantWarnings[0].message, `Creating a browser bundle that depends on Node.js built-in module ('util'). You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins` ); }); }); }); From ab14557a251cfa6a3c5971fe1a5182f46511c72d Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 16:03:46 -0500 Subject: [PATCH 06/28] lint --- src/ast/nodes/ExportDefaultDeclaration.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/ast/nodes/ExportDefaultDeclaration.js b/src/ast/nodes/ExportDefaultDeclaration.js index 208120f..7027291 100644 --- a/src/ast/nodes/ExportDefaultDeclaration.js +++ b/src/ast/nodes/ExportDefaultDeclaration.js @@ -1,6 +1,4 @@ import Node from '../Node.js'; -import { locate } from 'locate-character'; -import relativeId from '../../utils/relativeId.js'; const functionOrClassDeclaration = /^(?:Function|Class)Declaration/; From 881364117081d401ba4a2fd4cf7426781c5b3056 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 16:06:56 -0500 Subject: [PATCH 07/28] include location info in stringified warnings --- src/Bundle.js | 9 ++++++++- src/utils/defaults.js | 9 +++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/Bundle.js b/src/Bundle.js index f2f0dd2..520fa45 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -586,7 +586,14 @@ export default class Bundle { } warn ( warning ) { - warning.toString = () => warning.message || warning; + warning.toString = () => { + if ( warning.loc ) { + return `${warning.loc.file} (${warning.loc.line}:${warning.loc.column}) ${warning.message}`; + } + + return warning.message; + }; + this.onwarn( warning ); } } diff --git a/src/utils/defaults.js b/src/utils/defaults.js index 8db3c0b..c5dd054 100644 --- a/src/utils/defaults.js +++ b/src/utils/defaults.js @@ -45,9 +45,10 @@ export function resolveId ( importee, importer ) { export function makeOnwarn () { const warned = blank(); - return msg => { - if ( msg in warned ) return; - console.error( msg ); //eslint-disable-line no-console - warned[ msg ] = true; + return warning => { + const str = warning.toString(); + if ( str in warned ) return; + console.error( str ); //eslint-disable-line no-console + warned[ str ] = true; }; } From a44a171dadf2dfcb99cbe46fd4856be05d94241c Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 16:12:22 -0500 Subject: [PATCH 08/28] update CLI to handle new warnings --- bin/src/runRollup.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bin/src/runRollup.js b/bin/src/runRollup.js index 63bbd0c..baf8753 100644 --- a/bin/src/runRollup.js +++ b/bin/src/runRollup.js @@ -60,9 +60,8 @@ export default function runRollup ( command ) { rollup.rollup({ entry: config, onwarn: message => { - // TODO use warning codes instead of this hackery - if ( /treating it as an external dependency/.test( message ) ) return; - stderr( message ); + if ( message.code === 'UNRESOLVED_IMPORT' ) return; + stderr( message.toString() ); } }).then( bundle => { const { code } = bundle.generate({ @@ -121,7 +120,7 @@ function execute ( options, command ) { const optionsExternal = options.external; if ( command.globals ) { - let globals = Object.create( null ); + const globals = Object.create( null ); command.globals.split( ',' ).forEach( str => { const names = str.split( ':' ); @@ -144,7 +143,7 @@ function execute ( options, command ) { external = ( optionsExternal || [] ).concat( commandExternal ); } - options.onwarn = options.onwarn || stderr; + options.onwarn = options.onwarn || ( warning => stderr( warning.toString() ) ); options.external = external; From f695e628d6ee6011695b02d191e612b9ec9c2a89 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 16:49:34 -0500 Subject: [PATCH 09/28] richer warnings in CLI --- bin/src/runRollup.js | 29 ++++++++++++++++++++++++++++- rollup.config.cli.js | 4 +++- src/Bundle.js | 2 +- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/bin/src/runRollup.js b/bin/src/runRollup.js index baf8753..9ef3b19 100644 --- a/bin/src/runRollup.js +++ b/bin/src/runRollup.js @@ -1,7 +1,9 @@ import { realpathSync } from 'fs'; import * as rollup from 'rollup'; import relative from 'require-relative'; +import * as chalk from 'chalk'; import handleError from './handleError'; +import relativeId from '../../src/utils/relativeId.js'; import SOURCEMAPPING_URL from './sourceMappingUrl.js'; import { install as installSourcemapSupport } from 'source-map-support'; @@ -143,7 +145,32 @@ function execute ( options, command ) { external = ( optionsExternal || [] ).concat( commandExternal ); } - options.onwarn = options.onwarn || ( warning => stderr( warning.toString() ) ); + if ( !options.onwarn ) { + const seen = new Set(); + + options.onwarn = warning => { + const str = warning.toString(); + + if ( seen.has( str ) ) return; + seen.add( str ); + + stderr( `⚠️ ${chalk.bold( warning.message )}` ); + + if ( warning.url ) { + stderr( chalk.cyan( warning.url ) ); + } + + if ( warning.loc ) { + stderr( `${relativeId( warning.loc.file )} (${warning.loc.line}:${warning.loc.column})` ); + } + + if ( warning.frame ) { + stderr( chalk.dim( warning.frame ) ); + } + + stderr( '' ); + }; + } options.external = external; diff --git a/rollup.config.cli.js b/rollup.config.cli.js index 982f6b0..5c0f673 100644 --- a/rollup.config.cli.js +++ b/rollup.config.cli.js @@ -15,7 +15,9 @@ export default { buble(), commonjs({ include: 'node_modules/**', - namedExports: { chalk: [ 'red', 'cyan', 'grey' ] } + namedExports: { + chalk: [ 'yellow', 'red', 'cyan', 'grey', 'dim', 'bold' ] + } }), nodeResolve({ main: true diff --git a/src/Bundle.js b/src/Bundle.js index 520fa45..79cd8d0 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -588,7 +588,7 @@ export default class Bundle { warn ( warning ) { warning.toString = () => { if ( warning.loc ) { - return `${warning.loc.file} (${warning.loc.line}:${warning.loc.column}) ${warning.message}`; + return `${relativeId( warning.loc.file )} (${warning.loc.line}:${warning.loc.column}) ${warning.message}`; } return warning.message; From aa1c72a81c9aa518fcb3022535ac88b4c54023ea Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 17:19:45 -0500 Subject: [PATCH 10/28] -> v0.39.0 --- CHANGELOG.md | 4 ++++ package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51eca14..f2ae96c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # rollup changelog +## 0.39.0 + +* [BREAKING] Warnings are objects, rather than strings ([#1194](https://github.com/rollup/rollup/issues/1194)) + ## 0.38.3 * More informative warning for implicit external dependencies ([#1051](https://github.com/rollup/rollup/issues/1051)) diff --git a/package.json b/package.json index 4911fac..0708cef 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollup", - "version": "0.38.3", + "version": "0.39.0", "description": "Next-generation ES6 module bundler", "main": "dist/rollup.js", "module": "dist/rollup.es.js", From 979cfc25a725f39068f0978211a58529450fd856 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 17:32:54 -0500 Subject: [PATCH 11/28] update a few dependencies --- package.json | 10 +++++----- .../function/custom-external-resolver-async/_config.js | 1 - test/function/custom-path-resolver-async/_config.js | 1 - 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 0708cef..030a8ab 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,7 @@ "chalk": "^1.1.3", "codecov.io": "^0.1.6", "console-group": "^0.3.1", - "eslint": "^2.13.1", + "eslint": "^3.12.2", "eslint-plugin-import": "^2.2.0", "is-reference": "^1.0.0", "istanbul": "^0.4.3", @@ -59,14 +59,14 @@ "mocha": "^3.0.0", "remap-istanbul": "^0.6.4", "require-relative": "^0.8.7", - "rollup": "^0.34.0", - "rollup-plugin-buble": "^0.12.1", - "rollup-plugin-commonjs": "^3.0.0", + "rollup": "^0.39.0", + "rollup-plugin-buble": "^0.13.0", + "rollup-plugin-commonjs": "^7.0.0", "rollup-plugin-json": "^2.0.0", "rollup-plugin-node-resolve": "^2.0.0", "rollup-plugin-replace": "^1.1.0", "rollup-plugin-string": "^2.0.0", - "sander": "^0.5.1", + "sander": "^0.6.0", "source-map": "^0.5.6", "sourcemap-codec": "^1.3.0", "uglify-js": "^2.6.2" diff --git a/test/function/custom-external-resolver-async/_config.js b/test/function/custom-external-resolver-async/_config.js index 50e54fa..834f757 100644 --- a/test/function/custom-external-resolver-async/_config.js +++ b/test/function/custom-external-resolver-async/_config.js @@ -1,6 +1,5 @@ var path = require( 'path' ); var assert = require( 'assert' ); -var Promise = require( 'sander' ).Promise; module.exports = { description: 'uses a custom external path resolver (asynchronous)', diff --git a/test/function/custom-path-resolver-async/_config.js b/test/function/custom-path-resolver-async/_config.js index 3924db7..c471e02 100644 --- a/test/function/custom-path-resolver-async/_config.js +++ b/test/function/custom-path-resolver-async/_config.js @@ -6,7 +6,6 @@ module.exports = { options: { plugins: [{ resolveId: function ( importee, importer ) { - var Promise = require( 'sander' ).Promise; var resolved; if ( path.normalize(importee) === path.resolve( __dirname, 'main.js' ) ) return importee; From 708fb9821b877ddf36ac58ec928b2df285e7ccfd Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 17:43:51 -0500 Subject: [PATCH 12/28] warn on missing format (fixes #1197) --- src/Bundle.js | 6 ++---- src/rollup.js | 12 +++++++++++- test/cli/sourcemap-newline/_config.js | 2 +- test/test.js | 19 +++++++++++++++++++ 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/src/Bundle.js b/src/Bundle.js index 79cd8d0..a532b84 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -398,8 +398,6 @@ export default class Bundle { options.format = 'es'; } - const format = options.format || 'es'; - // Determine export mode - 'default', 'named', 'none' const exportMode = getExportMode( this, options ); @@ -409,7 +407,7 @@ export default class Bundle { timeStart( 'render modules' ); this.orderedModules.forEach( module => { - const source = module.render( format === 'es', this.legacy ); + const source = module.render( options.format === 'es', this.legacy ); if ( source.toString().length ) { magicString.addSource( source ); @@ -446,7 +444,7 @@ export default class Bundle { const indentString = getIndentString( magicString, options ); - const finalise = finalisers[ format ]; + const finalise = finalisers[ options.format ]; if ( !finalise ) throw new Error( `You must specify an output type - valid options are ${keys( finalisers ).join( ', ' )}` ); timeStart( 'render format' ); diff --git a/src/rollup.js b/src/rollup.js index dfb537d..c3e6379 100644 --- a/src/rollup.js +++ b/src/rollup.js @@ -67,7 +67,17 @@ export function rollup ( options ) { return bundle.build().then( () => { timeEnd( '--BUILD--' ); - function generate ( options ) { + function generate ( options = {} ) { + if ( !options.format ) { + bundle.warn({ + code: 'MISSING_FORMAT', + message: `No format option was supplied – defaulting to 'es'`, + url: `https://github.com/rollup/rollup/wiki/JavaScript-API#format` + }); + + options.format = 'es'; + } + timeStart( '--GENERATE--' ); const rendered = bundle.render( options ); diff --git a/test/cli/sourcemap-newline/_config.js b/test/cli/sourcemap-newline/_config.js index 1427619..8433548 100644 --- a/test/cli/sourcemap-newline/_config.js +++ b/test/cli/sourcemap-newline/_config.js @@ -2,7 +2,7 @@ const assert = require( 'assert' ); module.exports = { description: 'adds a newline after the sourceMappingURL comment (#756)', - command: 'rollup -i main.js -m inline', + command: 'rollup -i main.js -f es -m inline', result: code => { assert.equal( code.slice( -1 ), '\n' ); } diff --git a/test/test.js b/test/test.js index 789ac44..13f92e5 100644 --- a/test/test.js +++ b/test/test.js @@ -133,6 +133,25 @@ describe( 'rollup', function () { assert.ok( code[ code.length - 1 ] === '\n' ); }); }); + + it( 'warns on missing format option', () => { + const warnings = []; + + return rollup.rollup({ + entry: 'x', + plugins: [ loader({ x: `console.log( 42 );` }) ], + onwarn: warning => warnings.push( warning ) + }).then( bundle => { + bundle.generate(); + compareWarnings( warnings, [ + { + code: 'MISSING_FORMAT', + message: `No format option was supplied – defaulting to 'es'`, + url: `https://github.com/rollup/rollup/wiki/JavaScript-API#format` + } + ]); + }); + }); }); describe( 'bundle.write()', () => { From 532dff162efdd61070df805fc8b63fcb23bbf8c2 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 29 Dec 2016 18:45:33 -0500 Subject: [PATCH 13/28] change recommendation to local installation of rollup-watch (#1082) --- bin/src/handleError.js | 2 +- bin/src/runRollup.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/src/handleError.js b/bin/src/handleError.js index 80892fd..aff6cea 100644 --- a/bin/src/handleError.js +++ b/bin/src/handleError.js @@ -38,7 +38,7 @@ const handlers = { }, ROLLUP_WATCH_NOT_INSTALLED: () => { - stderr( chalk.red( 'rollup --watch depends on the rollup-watch package, which could not be found. You can install it globally (recommended) with ' ) + chalk.cyan( 'npm install -g rollup-watch' ) ); + stderr( chalk.red( 'rollup --watch depends on the rollup-watch package, which could not be found. Install it with ' ) + chalk.cyan( 'npm install -D rollup-watch' ) ); }, WATCHER_MISSING_INPUT_OR_OUTPUT: () => { diff --git a/bin/src/runRollup.js b/bin/src/runRollup.js index 9ef3b19..f511de1 100644 --- a/bin/src/runRollup.js +++ b/bin/src/runRollup.js @@ -193,7 +193,7 @@ function execute ( options, command ) { watcher.on( 'event', event => { switch ( event.code ) { - case 'STARTING': + case 'STARTING': // TODO this isn't emitted by newer versions of rollup-watch stderr( 'checking rollup-watch version...' ); break; From d1d667f9a8b1dda7ac69394d6e49a4691fda93aa Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Fri, 30 Dec 2016 11:34:35 -0500 Subject: [PATCH 14/28] ignore var inits in dead branches - fixes #1198 --- src/ast/nodes/IfStatement.js | 5 +++-- test/function/vars-with-init-in-dead-branch/_config.js | 9 +++++++++ test/function/vars-with-init-in-dead-branch/main.js | 4 ++++ 3 files changed, 16 insertions(+), 2 deletions(-) create mode 100644 test/function/vars-with-init-in-dead-branch/_config.js create mode 100644 test/function/vars-with-init-in-dead-branch/main.js diff --git a/src/ast/nodes/IfStatement.js b/src/ast/nodes/IfStatement.js index 87c8792..a16d3ca 100644 --- a/src/ast/nodes/IfStatement.js +++ b/src/ast/nodes/IfStatement.js @@ -17,9 +17,10 @@ function handleVarDeclarations ( node, scope ) { function visit ( node ) { if ( node.type === 'VariableDeclaration' && node.kind === 'var' ) { - node.initialise( scope ); - node.declarations.forEach( declarator => { + declarator.init = null; + declarator.initialise( scope ); + extractNames( declarator.id ).forEach( name => { if ( !~hoistedVars.indexOf( name ) ) hoistedVars.push( name ); }); diff --git a/test/function/vars-with-init-in-dead-branch/_config.js b/test/function/vars-with-init-in-dead-branch/_config.js new file mode 100644 index 0000000..c3a1c46 --- /dev/null +++ b/test/function/vars-with-init-in-dead-branch/_config.js @@ -0,0 +1,9 @@ +module.exports = { + description: 'handles vars with init in dead branch (#1198)', + warnings: [ + { + code: 'EMPTY_BUNDLE', + message: 'Generated an empty bundle' + } + ] +}; diff --git a/test/function/vars-with-init-in-dead-branch/main.js b/test/function/vars-with-init-in-dead-branch/main.js new file mode 100644 index 0000000..a2c1863 --- /dev/null +++ b/test/function/vars-with-init-in-dead-branch/main.js @@ -0,0 +1,4 @@ +if ( false ) { + var foo = []; + var bar = foo.concat( 'x' ); +} From 415985b1ee6264b4fdd1ba0d5744723a1d09739a Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Fri, 30 Dec 2016 11:45:57 -0500 Subject: [PATCH 15/28] -> v0.39.1 --- CHANGELOG.md | 4 ++++ package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f2ae96c..566c37c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # rollup changelog +## 0.39.1 + +* Ignore `var` initialisers in dead branches ([#1198](https://github.com/rollup/rollup/issues/1198)) + ## 0.39.0 * [BREAKING] Warnings are objects, rather than strings ([#1194](https://github.com/rollup/rollup/issues/1194)) diff --git a/package.json b/package.json index 030a8ab..e11afc6 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollup", - "version": "0.39.0", + "version": "0.39.1", "description": "Next-generation ES6 module bundler", "main": "dist/rollup.js", "module": "dist/rollup.es.js", From 595fa5878d7f0deba408adeab6283730591dd181 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Fri, 30 Dec 2016 12:24:53 -0500 Subject: [PATCH 16/28] prevent mutation of cached ASTs (#1153) --- src/Module.js | 14 +++++++++++--- src/ast/clone.js | 17 +++++++++++++++++ src/utils/object.js | 21 --------------------- test/test.js | 29 +++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 24 deletions(-) create mode 100644 src/ast/clone.js diff --git a/src/Module.js b/src/Module.js index 81926c3..f8e902d 100644 --- a/src/Module.js +++ b/src/Module.js @@ -2,7 +2,7 @@ import { parse } from 'acorn/src/index.js'; import MagicString from 'magic-string'; import { locate } from 'locate-character'; import { timeStart, timeEnd } from './utils/flushTime.js'; -import { assign, blank, deepClone, keys } from './utils/object.js'; +import { assign, blank, keys } from './utils/object.js'; import { basename, extname } from './utils/path.js'; import makeLegalIdentifier from './utils/makeLegalIdentifier.js'; import getCodeFrame from './utils/getCodeFrame.js'; @@ -12,6 +12,7 @@ import relativeId from './utils/relativeId.js'; import { SyntheticNamespaceDeclaration } from './Declaration.js'; import extractNames from './ast/utils/extractNames.js'; import enhance from './ast/enhance.js'; +import clone from './ast/clone.js'; import ModuleScope from './ast/scopes/ModuleScope.js'; function tryParse ( code, comments, acornOptions, id ) { @@ -41,8 +42,15 @@ export default class Module { timeStart( 'ast' ); - this.ast = ast || tryParse( code, this.comments, bundle.acornOptions, id ); // TODO what happens to comments if AST is provided? - this.astClone = deepClone( this.ast ); + if ( ast ) { + // prevent mutating the provided AST, as it may be reused on + // subsequent incremental rebuilds + this.ast = clone( ast ); + this.astClone = ast; + } else { + this.ast = tryParse( code, this.comments, bundle.acornOptions, id ); // TODO what happens to comments if AST is provided? + this.astClone = clone( this.ast ); + } timeEnd( 'ast' ); diff --git a/src/ast/clone.js b/src/ast/clone.js new file mode 100644 index 0000000..604b372 --- /dev/null +++ b/src/ast/clone.js @@ -0,0 +1,17 @@ +export default function clone ( node ) { + if ( !node ) return node; + if ( typeof node !== 'object' ) return node; + + if ( Array.isArray( node ) ) { + const cloned = new Array( node.length ); + for ( let i = 0; i < node.length; i += 1 ) cloned[i] = clone( node[i] ); + return cloned; + } + + const cloned = {}; + for ( const key in node ) { + cloned[ key ] = clone( node[ key ] ); + } + + return cloned; +} diff --git a/src/utils/object.js b/src/utils/object.js index 947c8e1..4234de3 100644 --- a/src/utils/object.js +++ b/src/utils/object.js @@ -17,24 +17,3 @@ export function assign ( target, ...sources ) { return target; } - -const isArray = Array.isArray; - -// used for cloning ASTs. Not for use with cyclical structures! -export function deepClone ( obj ) { - if ( !obj ) return obj; - if ( typeof obj !== 'object' ) return obj; - - if ( isArray( obj ) ) { - const clone = new Array( obj.length ); - for ( let i = 0; i < obj.length; i += 1 ) clone[i] = deepClone( obj[i] ); - return clone; - } - - const clone = {}; - for ( const key in obj ) { - clone[ key ] = deepClone( obj[ key ] ); - } - - return clone; -} diff --git a/test/test.js b/test/test.js index 13f92e5..78fead1 100644 --- a/test/test.js +++ b/test/test.js @@ -684,6 +684,35 @@ describe( 'rollup', function () { assert.deepEqual( asts.foo, acorn.parse( modules.foo, { sourceType: 'module' }) ); }); }); + + it( 'recovers from errors', () => { + modules.entry = `import foo from 'foo'; import bar from 'bar'; export default foo + bar;`; + + return rollup.rollup({ + entry: 'entry', + plugins: [ plugin ] + }).then( cache => { + modules.foo = `var 42 = nope;`; + + return rollup.rollup({ + entry: 'entry', + plugins: [ plugin ], + cache + }).catch( err => { + return cache; + }); + }).then( cache => { + modules.foo = `export default 42;`; + + return rollup.rollup({ + entry: 'entry', + plugins: [ plugin ], + cache + }).then( bundle => { + assert.equal( executeBundle( bundle ), 63 ); + }); + }); + }); }); describe( 'hooks', () => { From a00d3916c9e5b3d305cb2c628769ce4f86ada69c Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Fri, 30 Dec 2016 12:43:17 -0500 Subject: [PATCH 17/28] -> v0.39.2 --- CHANGELOG.md | 4 ++++ package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 566c37c..06b8d67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # rollup changelog +## 0.39.2 + +* Prevent mutation of cached ASTs (fixes stack overflow with rollup-watch) ([#1205](https://github.com/rollup/rollup/pull/1205)) + ## 0.39.1 * Ignore `var` initialisers in dead branches ([#1198](https://github.com/rollup/rollup/issues/1198)) diff --git a/package.json b/package.json index e11afc6..ff3982a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollup", - "version": "0.39.1", + "version": "0.39.2", "description": "Next-generation ES6 module bundler", "main": "dist/rollup.js", "module": "dist/rollup.es.js", From 23813bc4e73092ec40414d1fdfd09390bbb6c569 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Fri, 30 Dec 2016 13:23:39 -0500 Subject: [PATCH 18/28] apply kzc patch to prevent escape codes and emojis appearing in non-TTY stderr (#1201) --- bin/src/handleError.js | 2 +- bin/src/runRollup.js | 7 +++++-- rollup.config.cli.js | 5 +---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/bin/src/handleError.js b/bin/src/handleError.js index aff6cea..6882108 100644 --- a/bin/src/handleError.js +++ b/bin/src/handleError.js @@ -1,4 +1,4 @@ -import * as chalk from 'chalk'; +import chalk from 'chalk'; function stderr ( msg ) { console.error( msg ); // eslint-disable-line no-console diff --git a/bin/src/runRollup.js b/bin/src/runRollup.js index f511de1..2629d4b 100644 --- a/bin/src/runRollup.js +++ b/bin/src/runRollup.js @@ -1,7 +1,7 @@ import { realpathSync } from 'fs'; import * as rollup from 'rollup'; import relative from 'require-relative'; -import * as chalk from 'chalk'; +import chalk from 'chalk'; import handleError from './handleError'; import relativeId from '../../src/utils/relativeId.js'; import SOURCEMAPPING_URL from './sourceMappingUrl.js'; @@ -9,6 +9,9 @@ import SOURCEMAPPING_URL from './sourceMappingUrl.js'; import { install as installSourcemapSupport } from 'source-map-support'; installSourcemapSupport(); +if ( !process.stderr.isTTY ) chalk.enabled = false; +const warnSymbol = process.stderr.isTTY ? `⚠️ ` : `Warning: `; + // stderr to stderr to keep `rollup main.js > bundle.js` from breaking const stderr = console.error.bind( console ); // eslint-disable-line no-console @@ -154,7 +157,7 @@ function execute ( options, command ) { if ( seen.has( str ) ) return; seen.add( str ); - stderr( `⚠️ ${chalk.bold( warning.message )}` ); + stderr( `${warnSymbol}${chalk.bold( warning.message )}` ); if ( warning.url ) { stderr( chalk.cyan( warning.url ) ); diff --git a/rollup.config.cli.js b/rollup.config.cli.js index 5c0f673..d8dae2a 100644 --- a/rollup.config.cli.js +++ b/rollup.config.cli.js @@ -14,10 +14,7 @@ export default { json(), buble(), commonjs({ - include: 'node_modules/**', - namedExports: { - chalk: [ 'yellow', 'red', 'cyan', 'grey', 'dim', 'bold' ] - } + include: 'node_modules/**' }), nodeResolve({ main: true From 812d1062baf657c18c412ee722667a7b7972a359 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Sat, 31 Dec 2016 12:56:31 -0500 Subject: [PATCH 19/28] chmod a+x bin/rollup (#1209) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index ff3982a..b7a285d 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "posttest-coverage": "remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped.json -b dist && remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped.lcov -t lcovonly -b dist && remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped -t html -b dist", "ci": "npm run test-coverage && codecov < coverage/coverage-remapped.lcov", "build": "git rev-parse HEAD > .commithash && rollup -c", - "build:cli": "rollup -c rollup.config.cli.js", + "build:cli": "rollup -c rollup.config.cli.js && chmod a+x bin/rollup", "build:browser": "git rev-parse HEAD > .commithash && rollup -c rollup.config.browser.js", "watch": "rollup -c -w", "watch:browser": "rollup -c rollup.config.browser.js -w", From be0f82c6238ea3d83544df1e309a2b7c78331641 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Mon, 2 Jan 2017 09:24:50 -0500 Subject: [PATCH 20/28] add module.error helper for consistently reporting errors (#545) --- src/Bundle.js | 24 ++++++++++- src/Module.js | 43 ++++++++++++------- src/ast/nodes/CallExpression.js | 10 ++--- .../cannot-call-external-namespace/_config.js | 19 +++++--- .../cannot-call-internal-namespace/_config.js | 19 +++++--- test/function/cannot-import-self/_config.js | 16 ++++++- .../check-resolve-for-entry/_config.js | 5 ++- .../custom-path-resolver-plural-b/_config.js | 10 ++--- .../default-not-reexported/_config.js | 19 +++++++- .../function/double-default-export/_config.js | 16 ++++++- test/function/double-named-export/_config.js | 17 +++++++- .../function/double-named-reexport/_config.js | 17 +++++++- .../export-not-at-top-level-fails/_config.js | 19 ++++++-- .../import-not-at-top-level-fails/_config.js | 19 ++++++-- .../import-not-at-top-level-fails/main.js | 2 +- .../import-of-unexported-fails/_config.js | 19 +++++++- .../reports-syntax-error-locations/_config.js | 8 ---- .../reports-syntax-error-locations/main.js | 1 - test/test.js | 23 +++++++++- 19 files changed, 235 insertions(+), 71 deletions(-) delete mode 100644 test/function/reports-syntax-error-locations/_config.js delete mode 100644 test/function/reports-syntax-error-locations/main.js diff --git a/src/Bundle.js b/src/Bundle.js index a532b84..5489797 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -17,6 +17,7 @@ import transformBundle from './utils/transformBundle.js'; import collapseSourcemaps from './utils/collapseSourcemaps.js'; import callIfFunction from './utils/callIfFunction.js'; import relativeId from './utils/relativeId.js'; +import error from './utils/error.js'; import { dirname, isRelative, isAbsolute, normalize, relative, resolve } from './utils/path.js'; import BundleScope from './ast/scopes/BundleScope.js'; @@ -106,7 +107,13 @@ export default class Bundle { // of the entry module's dependencies return this.resolveId( this.entry, undefined ) .then( id => { - if ( id == null ) throw new Error( `Could not resolve entry (${this.entry})` ); + if ( id == null ) { + error({ + code: 'UNRESOLVED_ENTRY', + message: `Could not resolve entry (${this.entry})` + }); + } + this.entryId = id; return this.fetchModule( id, undefined ); }) @@ -367,7 +374,20 @@ export default class Bundle { }); } else { if ( resolvedId === module.id ) { - throw new Error( `A module cannot import itself (${resolvedId})` ); + // need to find the actual import declaration, so we can provide + // a useful error message. Bit hoop-jumpy but what can you do + const name = Object.keys( module.imports ) + .find( name => { + const declaration = module.imports[ name ]; + return declaration.source === source; + }); + + const declaration = module.imports[ name ].specifier.parent; + + module.error({ + code: 'CANNOT_IMPORT_SELF', + message: `A module cannot import itself` + }, declaration.start ); } module.resolvedIds[ source ] = resolvedId; diff --git a/src/Module.js b/src/Module.js index f8e902d..b16f266 100644 --- a/src/Module.js +++ b/src/Module.js @@ -15,25 +15,27 @@ import enhance from './ast/enhance.js'; import clone from './ast/clone.js'; import ModuleScope from './ast/scopes/ModuleScope.js'; -function tryParse ( code, comments, acornOptions, id ) { +function tryParse ( module, acornOptions ) { try { - return parse( code, assign({ + return parse( module.code, assign({ ecmaVersion: 8, sourceType: 'module', - onComment: ( block, text, start, end ) => comments.push({ block, text, start, end }), + onComment: ( block, text, start, end ) => module.comments.push({ block, text, start, end }), preserveParens: false }, acornOptions )); } catch ( err ) { - err.code = 'PARSE_ERROR'; - err.file = id; // see above - not necessarily true, but true enough - err.message += ` in ${id}`; - throw err; + module.error({ + code: 'PARSE_ERROR', + message: err.message.replace( / \(\d+:\d+\)$/, '' ) + }, err.pos ); } } export default class Module { constructor ({ id, code, originalCode, originalSourceMap, ast, sourceMapChain, resolvedIds, bundle }) { this.code = code; + this.id = id; + this.bundle = bundle; this.originalCode = originalCode; this.originalSourceMap = originalSourceMap; this.sourceMapChain = sourceMapChain; @@ -48,14 +50,12 @@ export default class Module { this.ast = clone( ast ); this.astClone = ast; } else { - this.ast = tryParse( code, this.comments, bundle.acornOptions, id ); // TODO what happens to comments if AST is provided? + this.ast = tryParse( this, bundle.acornOptions ); // TODO what happens to comments if AST is provided? this.astClone = clone( this.ast ); } timeEnd( 'ast' ); - this.bundle = bundle; - this.id = id; this.excludeFromSourcemap = /\0/.test( id ); this.context = bundle.getModuleContext( id ); @@ -282,6 +282,19 @@ export default class Module { // } } + error ( props, pos ) { + if ( pos !== undefined ) { + props.pos = pos; + + const { line, column } = locate( this.code, pos, { offsetLine: 1 }); // TODO trace sourcemaps + + props.loc = { file: this.id, line, column }; + props.frame = getCodeFrame( this.code, line, column ); + } + + error( props ); + } + findParent () { // TODO what does it mean if we're here? return null; @@ -372,11 +385,11 @@ export default class Module { const declaration = otherModule.traceExport( importDeclaration.name ); if ( !declaration ) { - error({ - message: `'${importDeclaration.name}' is not exported by ${relativeId( otherModule.id )} (imported by ${relativeId( this.id )}). For help fixing this error see https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module`, - file: this.id, - loc: locate( this.code, importDeclaration.specifier.start, { offsetLine: 1 }) - }); + this.error({ + code: 'MISSING_EXPORT', + message: `'${importDeclaration.name}' is not exported by ${relativeId( otherModule.id )}`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` + }, importDeclaration.specifier.start ); } return declaration; diff --git a/src/ast/nodes/CallExpression.js b/src/ast/nodes/CallExpression.js index 4880cb5..dc3e6b5 100644 --- a/src/ast/nodes/CallExpression.js +++ b/src/ast/nodes/CallExpression.js @@ -10,12 +10,10 @@ export default class CallExpression extends Node { const declaration = scope.findDeclaration( this.callee.name ); if ( declaration.isNamespace ) { - error({ - message: `Cannot call a namespace ('${this.callee.name}')`, - file: this.module.id, - pos: this.start, - loc: locate( this.module.code, this.start, { offsetLine: 1 }) - }); + this.module.error({ + code: 'CANNOT_CALL_NAMESPACE', + message: `Cannot call a namespace ('${this.callee.name}')` + }, this.start ); } if ( this.callee.name === 'eval' && declaration.isGlobal ) { diff --git a/test/function/cannot-call-external-namespace/_config.js b/test/function/cannot-call-external-namespace/_config.js index 5ed02e2..a891b1f 100644 --- a/test/function/cannot-call-external-namespace/_config.js +++ b/test/function/cannot-call-external-namespace/_config.js @@ -3,10 +3,19 @@ var assert = require( 'assert' ); module.exports = { description: 'errors if code calls an external namespace', - error: function ( err ) { - assert.equal( err.message, 'Cannot call a namespace (\'foo\')' ); - assert.equal( err.file.replace( /\//g, path.sep ), path.resolve( __dirname, 'main.js' ) ); - assert.equal( err.pos, 28 ); - assert.deepEqual( err.loc, { character: 28, line: 2, column: 0 }); + error: { + code: 'CANNOT_CALL_NAMESPACE', + message: `Cannot call a namespace ('foo')`, + pos: 28, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 2, + column: 0 + }, + frame: ` + 1: import * as foo from 'foo'; + 2: foo(); + ^ + ` } }; diff --git a/test/function/cannot-call-internal-namespace/_config.js b/test/function/cannot-call-internal-namespace/_config.js index d347008..6067234 100644 --- a/test/function/cannot-call-internal-namespace/_config.js +++ b/test/function/cannot-call-internal-namespace/_config.js @@ -3,10 +3,19 @@ var assert = require( 'assert' ); module.exports = { description: 'errors if code calls an internal namespace', - error: function ( err ) { - assert.equal( err.message, 'Cannot call a namespace (\'foo\')' ); - assert.equal( err.file.replace( /\//g, path.sep ), path.resolve( __dirname, 'main.js' ) ); - assert.equal( err.pos, 33 ); - assert.deepEqual( err.loc, { character: 33, line: 2, column: 0 }); + error: { + code: 'CANNOT_CALL_NAMESPACE', + message: `Cannot call a namespace ('foo')`, + pos: 33, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 2, + column: 0 + }, + frame: ` + 1: import * as foo from './foo.js'; + 2: foo(); + ^ + ` } }; diff --git a/test/function/cannot-import-self/_config.js b/test/function/cannot-import-self/_config.js index 2413d03..f2896e9 100644 --- a/test/function/cannot-import-self/_config.js +++ b/test/function/cannot-import-self/_config.js @@ -1,8 +1,20 @@ +var path = require( 'path' ); var assert = require( 'assert' ); module.exports = { description: 'prevents a module importing itself', - error: function ( err ) { - assert.ok( /A module cannot import itself/.test( err.message ) ); + error: { + code: 'CANNOT_IMPORT_SELF', + message: `A module cannot import itself`, + pos: 0, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 1, + column: 0 + }, + frame: ` + 1: import me from './main'; + ^ + ` } }; diff --git a/test/function/check-resolve-for-entry/_config.js b/test/function/check-resolve-for-entry/_config.js index a06632d..cd3da5f 100644 --- a/test/function/check-resolve-for-entry/_config.js +++ b/test/function/check-resolve-for-entry/_config.js @@ -5,7 +5,8 @@ module.exports = { options: { entry: '/not/a/path/that/actually/really/exists' }, - error: function ( err ) { - assert.ok( /Could not resolve entry/.test( err.message ) ); + error: { + code: 'UNRESOLVED_ENTRY', + message: 'Could not resolve entry (/not/a/path/that/actually/really/exists)' } }; diff --git a/test/function/custom-path-resolver-plural-b/_config.js b/test/function/custom-path-resolver-plural-b/_config.js index cb2da0c..7ca6fe6 100644 --- a/test/function/custom-path-resolver-plural-b/_config.js +++ b/test/function/custom-path-resolver-plural-b/_config.js @@ -5,21 +5,21 @@ module.exports = { options: { plugins: [ { - resolveId: function () { + resolveId () { throw new Error( 'nope' ); }, - load: function ( id ) { + load ( id ) { if ( id === 'main' ) return 'assert.ok( false );'; } }, { - resolveId: function ( importee, importer ) { + resolveId ( importee, importer ) { return 'main'; } } ] }, - error: function ( err ) { - assert.equal( err.message, 'nope' ); + error: { + message: 'nope' } }; diff --git a/test/function/default-not-reexported/_config.js b/test/function/default-not-reexported/_config.js index 4280924..3bd95b8 100644 --- a/test/function/default-not-reexported/_config.js +++ b/test/function/default-not-reexported/_config.js @@ -1,8 +1,23 @@ +const path = require( 'path' ); const assert = require( 'assert' ); module.exports = { description: 'default export is not re-exported with export *', - error ( error ) { - assert.equal( error.message, `'default' is not exported by foo.js (imported by main.js). For help fixing this error see https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` ); + error: { + code: 'MISSING_EXPORT', + message: `'default' is not exported by foo.js`, + pos: 7, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 1, + column: 7 + }, + frame: ` + 1: import def from './foo.js'; + ^ + 2: + 3: console.log( def ); + `, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` } }; diff --git a/test/function/double-default-export/_config.js b/test/function/double-default-export/_config.js index d5f5254..3a9f85f 100644 --- a/test/function/double-default-export/_config.js +++ b/test/function/double-default-export/_config.js @@ -3,7 +3,19 @@ const assert = require( 'assert' ); module.exports = { description: 'throws on double default exports', - error: err => { - assert.equal( err.message, `Duplicate export 'default' (2:7) in ${path.resolve(__dirname, 'foo.js')}` ); + error: { + code: 'PARSE_ERROR', + message: `Duplicate export 'default'`, + pos: 25, + loc: { + file: path.resolve( __dirname, 'foo.js' ), + line: 2, + column: 7 + }, + frame: ` + 1: export default 1; + 2: export default 2; + ^ + ` } }; diff --git a/test/function/double-named-export/_config.js b/test/function/double-named-export/_config.js index 169ce76..0982751 100644 --- a/test/function/double-named-export/_config.js +++ b/test/function/double-named-export/_config.js @@ -3,7 +3,20 @@ const assert = require( 'assert' ); module.exports = { description: 'throws on duplicate named exports', - error: err => { - assert.equal( err.message, `Duplicate export 'foo' (3:9) in ${path.resolve(__dirname, 'foo.js')}` ); + error: { + code: 'PARSE_ERROR', + message: `Duplicate export 'foo'`, + pos: 38, + loc: { + file: path.resolve( __dirname, 'foo.js' ), + line: 3, + column: 9 + }, + frame: ` + 1: var foo = 1; + 2: export { foo }; + 3: export { foo }; + ^ + ` } }; diff --git a/test/function/double-named-reexport/_config.js b/test/function/double-named-reexport/_config.js index 169ce76..1c7ff1c 100644 --- a/test/function/double-named-reexport/_config.js +++ b/test/function/double-named-reexport/_config.js @@ -3,7 +3,20 @@ const assert = require( 'assert' ); module.exports = { description: 'throws on duplicate named exports', - error: err => { - assert.equal( err.message, `Duplicate export 'foo' (3:9) in ${path.resolve(__dirname, 'foo.js')}` ); + error: { + code: 'PARSE_ERROR', + message: `Duplicate export 'foo'`, + pos: 38, + loc: { + file: path.resolve( __dirname, 'foo.js' ), + line: 3, + column: 9 + }, + frame: ` + 1: var foo = 1; + 2: export { foo }; + 3: export { foo } from './bar.js'; + ^ + ` } }; diff --git a/test/function/export-not-at-top-level-fails/_config.js b/test/function/export-not-at-top-level-fails/_config.js index 2dadeab..7fc1e3f 100644 --- a/test/function/export-not-at-top-level-fails/_config.js +++ b/test/function/export-not-at-top-level-fails/_config.js @@ -3,9 +3,20 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows non-top-level exports', - error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 2, column: 2 }); - assert.ok( /may only appear at the top level/.test( err.message ) ); + error: { + code: 'PARSE_ERROR', + message: `'import' and 'export' may only appear at the top level`, + pos: 19, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 2, + column: 2 + }, + frame: ` + 1: function foo() { + 2: export { foo }; + ^ + 3: } + ` } }; diff --git a/test/function/import-not-at-top-level-fails/_config.js b/test/function/import-not-at-top-level-fails/_config.js index 4f873aa..4a68cc6 100644 --- a/test/function/import-not-at-top-level-fails/_config.js +++ b/test/function/import-not-at-top-level-fails/_config.js @@ -3,9 +3,20 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows non-top-level imports', - error: function ( err ) { - assert.equal( path.normalize(err.file), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { line: 2, column: 2 }); - assert.ok( /may only appear at the top level/.test( err.message ) ); + error: { + code: 'PARSE_ERROR', + message: `'import' and 'export' may only appear at the top level`, + pos: 19, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 2, + column: 2 + }, + frame: ` + 1: function foo() { + 2: import foo from './foo.js'; + ^ + 3: } + ` } }; diff --git a/test/function/import-not-at-top-level-fails/main.js b/test/function/import-not-at-top-level-fails/main.js index 1fa68e1..c88024f 100644 --- a/test/function/import-not-at-top-level-fails/main.js +++ b/test/function/import-not-at-top-level-fails/main.js @@ -1,3 +1,3 @@ function foo() { - import foo from './foo'; + import foo from './foo.js'; } diff --git a/test/function/import-of-unexported-fails/_config.js b/test/function/import-of-unexported-fails/_config.js index 10d11a6..3581bd9 100644 --- a/test/function/import-of-unexported-fails/_config.js +++ b/test/function/import-of-unexported-fails/_config.js @@ -1,8 +1,23 @@ +var path = require( 'path' ); var assert = require( 'assert' ); module.exports = { description: 'marking an imported, but unexported, identifier should throw', - error: function ( err ) { - assert.equal( err.message, `'default' is not exported by empty.js (imported by main.js). For help fixing this error see https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` ); + error: { + code: 'MISSING_EXPORT', + message: `'default' is not exported by empty.js`, + pos: 7, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 1, + column: 7 + }, + frame: ` + 1: import a from './empty.js'; + ^ + 2: + 3: a(); + `, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` } }; diff --git a/test/function/reports-syntax-error-locations/_config.js b/test/function/reports-syntax-error-locations/_config.js deleted file mode 100644 index 2cb1085..0000000 --- a/test/function/reports-syntax-error-locations/_config.js +++ /dev/null @@ -1,8 +0,0 @@ -var assert = require( 'assert' ); - -module.exports = { - description: 'reports syntax error filename', - error: function ( err ) { - assert.ok( /in .+main\.js/.test( err.message ) ); - } -}; diff --git a/test/function/reports-syntax-error-locations/main.js b/test/function/reports-syntax-error-locations/main.js deleted file mode 100644 index d24584b..0000000 --- a/test/function/reports-syntax-error-locations/main.js +++ /dev/null @@ -1 +0,0 @@ -var 42 = answer; diff --git a/test/test.js b/test/test.js index 78fead1..7401b66 100644 --- a/test/test.js +++ b/test/test.js @@ -79,6 +79,23 @@ function compareWarnings ( actual, expected ) { ); } +function compareError ( actual, expected ) { + delete actual.stack; + actual = Object.assign( {}, actual, { + message: actual.message + }); + + if ( actual.frame ) { + actual.frame = actual.frame.replace( /\s+$/gm, '' ); + } + + if ( expected.frame ) { + expected.frame = expected.frame.slice( 1 ).replace( /^\t+/gm, '' ).replace( /\s+$/gm, '' ).trim(); + } + + assert.deepEqual( actual, expected ); +} + describe( 'rollup', function () { this.timeout( 10000 ); @@ -324,7 +341,11 @@ describe( 'rollup', function () { if ( unintendedError ) throw unintendedError; }, err => { if ( config.error ) { - config.error( err ); + if ( typeof config.error === 'object' ) { + compareError( err, config.error ); + } else { + config.error( err ); + } } else { throw err; } From 2368ae242e73ceb41e9dcbc3fc6fe1cdeb76cf14 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Mon, 2 Jan 2017 10:37:34 -0500 Subject: [PATCH 21/28] use error helpers consistently, update tests --- src/Bundle.js | 13 ++++++++-- src/Module.js | 18 ++++++------- .../shared/disallowIllegalReassignment.js | 23 ++++++----------- src/utils/getCodeFrame.js | 11 +++++--- src/utils/transform.js | 25 +++++++++++++------ .../duplicate-import-fails/_config.js | 20 ++++++++++++--- .../_config.js | 18 ++++++++++--- .../load-returns-string-or-null/_config.js | 6 +++-- .../_config.js | 19 +++++++++++--- .../namespace-update-import-fails/_config.js | 19 +++++++++++--- test/function/no-relative-external/_config.js | 5 ++-- .../paths-are-case-sensitive/_config.js | 5 ++-- .../function/reassign-import-fails/_config.js | 19 +++++++++++--- .../_config.js | 20 ++++++++++++--- .../reexport-missing-error/_config.js | 17 +++++++++++-- .../report-transform-error-file/_config.js | 9 +++++-- .../throws-not-found-module/_config.js | 7 +++--- .../throws-only-first-transform/_config.js | 21 ++++++++-------- .../_config.js | 19 +++++++++++--- test/test.js | 6 +---- 20 files changed, 205 insertions(+), 95 deletions(-) diff --git a/src/Bundle.js b/src/Bundle.js index 5489797..c945184 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -275,7 +275,11 @@ export default class Bundle { if ( typeof source === 'string' ) return source; if ( source && typeof source === 'object' && source.code ) return source; - throw new Error( `Error loading ${id}: load hook should return a string, a { code, map } object, or nothing/null` ); + // TODO report which plugin failed + error({ + code: 'BAD_LOADER', + message: `Error loading ${relativeId( id )}: plugin load hook should return a string, a { code, map } object, or nothing/null` + }); }) .then( source => { if ( typeof source === 'string' ) { @@ -344,7 +348,12 @@ export default class Bundle { let isExternal = this.isExternal( externalId ); if ( !resolvedId && !isExternal ) { - if ( isRelative( source ) ) throw new Error( `Could not resolve '${source}' from ${module.id}` ); + if ( isRelative( source ) ) { + error({ + code: 'UNRESOLVED_IMPORT', + message: `Could not resolve '${source}' from ${relativeId( module.id )}` + }); + } this.warn({ code: 'UNRESOLVED_IMPORT', diff --git a/src/Module.js b/src/Module.js index b16f266..dadd5a5 100644 --- a/src/Module.js +++ b/src/Module.js @@ -207,10 +207,10 @@ export default class Module { const localName = specifier.local.name; if ( this.imports[ localName ] ) { - const err = new Error( `Duplicated import '${localName}'` ); - err.file = this.id; - err.loc = locate( this.code, specifier.start, { offsetLine: 1 }); - throw err; + this.error({ + code: 'DUPLICATE_IMPORT', + message: `Duplicated import '${localName}'` + }, specifier.start ); } const isDefault = specifier.type === 'ImportDefaultSpecifier'; @@ -405,11 +405,11 @@ export default class Module { const declaration = reexportDeclaration.module.traceExport( reexportDeclaration.localName ); if ( !declaration ) { - error({ - message: `'${reexportDeclaration.localName}' is not exported by '${reexportDeclaration.module.id}' (imported by '${this.id}')`, - file: this.id, - loc: locate( this.code, reexportDeclaration.start, { offsetLine: 1 }) - }); + this.error({ + code: 'MISSING_EXPORT', + message: `'${reexportDeclaration.localName}' is not exported by ${relativeId( reexportDeclaration.module.id )}`, + url: `https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module` + }, reexportDeclaration.start ); } return declaration; diff --git a/src/ast/nodes/shared/disallowIllegalReassignment.js b/src/ast/nodes/shared/disallowIllegalReassignment.js index 5d338af..17d054f 100644 --- a/src/ast/nodes/shared/disallowIllegalReassignment.js +++ b/src/ast/nodes/shared/disallowIllegalReassignment.js @@ -1,28 +1,21 @@ -import { locate } from 'locate-character'; -import error from '../../../utils/error.js'; - // TODO tidy this up a bit (e.g. they can both use node.module.imports) export default function disallowIllegalReassignment ( scope, node ) { if ( node.type === 'MemberExpression' && node.object.type === 'Identifier' ) { const declaration = scope.findDeclaration( node.object.name ); if ( declaration.isNamespace ) { - error({ - message: `Illegal reassignment to import '${node.object.name}'`, - file: node.module.id, - pos: node.start, - loc: locate( node.module.code, node.start, { offsetLine: 1 }) - }); + node.module.error({ + code: 'ILLEGAL_NAMESPACE_REASSIGNMENT', + message: `Illegal reassignment to import '${node.object.name}'` + }, node.start ); } } else if ( node.type === 'Identifier' ) { if ( node.module.imports[ node.name ] && !scope.contains( node.name ) ) { - error({ - message: `Illegal reassignment to import '${node.name}'`, - file: node.module.id, - pos: node.start, - loc: locate( node.module.code, node.start, { offsetLine: 1 }) - }); + node.module.error({ + code: 'ILLEGAL_REASSIGNMENT', + message: `Illegal reassignment to import '${node.name}'` + }, node.start ); } } } diff --git a/src/utils/getCodeFrame.js b/src/utils/getCodeFrame.js index 369829b..d5aa7a3 100644 --- a/src/utils/getCodeFrame.js +++ b/src/utils/getCodeFrame.js @@ -13,12 +13,15 @@ export default function getCodeFrame ( source, line, column ) { let lines = source.split( '\n' ); const frameStart = Math.max( 0, line - 3 ); - const frameEnd = Math.min( line + 2, lines.length ); - - const digits = String( frameEnd + 1 ).length; + let frameEnd = Math.min( line + 2, lines.length ); lines = lines.slice( frameStart, frameEnd ); - while ( !/\S/.test( lines[ lines.length - 1 ] ) ) lines.pop(); + while ( !/\S/.test( lines[ lines.length - 1 ] ) ) { + lines.pop(); + frameEnd -= 1; + } + + const digits = String( frameEnd ).length; return lines .map( ( str, i ) => { diff --git a/src/utils/transform.js b/src/utils/transform.js index 2f0c167..7e90a74 100644 --- a/src/utils/transform.js +++ b/src/utils/transform.js @@ -1,4 +1,6 @@ import { decode } from 'sourcemap-codec'; +import error from './error.js'; +import relativeId from './relativeId.js'; export default function transform ( source, id, plugins ) { const sourceMapChain = []; @@ -11,6 +13,7 @@ export default function transform ( source, id, plugins ) { const originalCode = source.code; let ast = source.ast; + let errored = false; return plugins.reduce( ( promise, plugin ) => { return promise.then( previous => { @@ -41,15 +44,21 @@ export default function transform ( source, id, plugins ) { return result.code; }); }).catch( err => { - if ( !err.rollupTransform ) { - err.rollupTransform = true; - err.id = id; - err.plugin = plugin.name; - err.message = `Error transforming ${id}${plugin.name ? ` with '${plugin.name}' plugin` : ''}: ${err.message}`; - } + // TODO this all seems a bit hacky + if ( errored ) throw err; + errored = true; + + err.plugin = plugin.name; throw err; }); }, Promise.resolve( source.code ) ) - - .then( code => ({ code, originalCode, originalSourceMap, ast, sourceMapChain }) ); + .catch( err => { + error({ + code: 'BAD_TRANSFORMER', + message: `Error transforming ${relativeId( id )}${err.plugin ? ` with '${err.plugin}' plugin` : ''}: ${err.message}`, + plugin: err.plugin, + id + }); + }) + .then( code => ({ code, originalCode, originalSourceMap, ast, sourceMapChain }) ); } diff --git a/test/function/duplicate-import-fails/_config.js b/test/function/duplicate-import-fails/_config.js index 492ac2f..f47de60 100644 --- a/test/function/duplicate-import-fails/_config.js +++ b/test/function/duplicate-import-fails/_config.js @@ -3,10 +3,22 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows duplicate imports', - error: function ( err ) { - assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { character: 36, line: 2, column: 9 }); - assert.ok( /Duplicated import/.test( err.message ) ); + error: { + code: 'DUPLICATE_IMPORT', + message: `Duplicated import 'a'`, + pos: 36, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 2, + column: 9 + }, + frame: ` + 1: import { a } from './foo'; + 2: import { a } from './foo'; + ^ + 3: + 4: assert.equal(a, 1); + ` } }; diff --git a/test/function/duplicate-import-specifier-fails/_config.js b/test/function/duplicate-import-specifier-fails/_config.js index 58446af..35c88ce 100644 --- a/test/function/duplicate-import-specifier-fails/_config.js +++ b/test/function/duplicate-import-specifier-fails/_config.js @@ -3,10 +3,20 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows duplicate import specifiers', - error: function ( err ) { - assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { character: 12, line: 1, column: 12 }); - assert.ok( /Duplicated import/.test( err.message ) ); + error: { + code: 'DUPLICATE_IMPORT', + message: `Duplicated import 'a'`, + pos: 12, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 1, + column: 12 + }, + frame: ` + 1: import { a, a } from './foo'; + ^ + 2: assert.equal(a, 1); + ` } }; diff --git a/test/function/load-returns-string-or-null/_config.js b/test/function/load-returns-string-or-null/_config.js index ea80699..b49e3eb 100644 --- a/test/function/load-returns-string-or-null/_config.js +++ b/test/function/load-returns-string-or-null/_config.js @@ -4,12 +4,14 @@ module.exports = { description: 'throws error if load returns something wacky', options: { plugins: [{ + name: 'bad-plugin', load: function () { return 42; } }] }, - error: function ( err ) { - assert.ok( /load hook should return a string, a \{ code, map \} object, or nothing\/null/.test( err.message ) ); + error: { + code: 'BAD_LOADER', + message: `Error loading main.js: plugin load hook should return a string, a { code, map } object, or nothing/null` } }; diff --git a/test/function/namespace-reassign-import-fails/_config.js b/test/function/namespace-reassign-import-fails/_config.js index a76a36a..97335b1 100644 --- a/test/function/namespace-reassign-import-fails/_config.js +++ b/test/function/namespace-reassign-import-fails/_config.js @@ -3,10 +3,21 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows reassignments to namespace exports', - error: function ( err ) { - assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { character: 31, line: 3, column: 0 }); - assert.ok( /Illegal reassignment/.test( err.message ) ); + error: { + code: 'ILLEGAL_NAMESPACE_REASSIGNMENT', + message: `Illegal reassignment to import 'exp'`, + pos: 31, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 3, + column: 0 + }, + frame: ` + 1: import * as exp from './foo'; + 2: + 3: exp.foo = 2; + ^ + ` } }; diff --git a/test/function/namespace-update-import-fails/_config.js b/test/function/namespace-update-import-fails/_config.js index dc8e0f3..33ea524 100644 --- a/test/function/namespace-update-import-fails/_config.js +++ b/test/function/namespace-update-import-fails/_config.js @@ -3,10 +3,21 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows updates to namespace exports', - error: function ( err ) { - assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { character: 31, line: 3, column: 0 }); - assert.ok( /Illegal reassignment/.test( err.message ) ); + error: { + code: 'ILLEGAL_NAMESPACE_REASSIGNMENT', + message: `Illegal reassignment to import 'exp'`, + pos: 31, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 3, + column: 0 + }, + frame: ` + 1: import * as exp from './foo'; + 2: + 3: exp['foo']++; + ^ + ` } }; diff --git a/test/function/no-relative-external/_config.js b/test/function/no-relative-external/_config.js index b5c37e0..b87c9a1 100644 --- a/test/function/no-relative-external/_config.js +++ b/test/function/no-relative-external/_config.js @@ -2,7 +2,8 @@ var assert = require( 'assert' ); module.exports = { description: 'missing relative imports are an error, not a warning', - error: function ( err ) { - assert.ok( /Could not resolve '\.\/missing\.js' from/.test( err.message ) ); + error: { + code: 'UNRESOLVED_IMPORT', + message: `Could not resolve './missing.js' from main.js` } }; diff --git a/test/function/paths-are-case-sensitive/_config.js b/test/function/paths-are-case-sensitive/_config.js index 3f81b80..5b62cf7 100644 --- a/test/function/paths-are-case-sensitive/_config.js +++ b/test/function/paths-are-case-sensitive/_config.js @@ -2,7 +2,8 @@ var assert = require( 'assert' ); module.exports = { description: 'insists on correct casing for imports', - error: function ( err ) { - assert.ok( /Could not resolve/.test( err.message ) ); + error: { + code: 'UNRESOLVED_IMPORT', + message: `Could not resolve './foo.js' from main.js` } }; diff --git a/test/function/reassign-import-fails/_config.js b/test/function/reassign-import-fails/_config.js index 4854d02..922c41b 100644 --- a/test/function/reassign-import-fails/_config.js +++ b/test/function/reassign-import-fails/_config.js @@ -3,10 +3,21 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows assignments to imported bindings', - error: function ( err ) { - assert.ok( /Illegal reassignment/.test( err.message ) ); - assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { character: 113, line: 8, column: 0 }); + error: { + code: 'ILLEGAL_REASSIGNMENT', + message: `Illegal reassignment to import 'x'`, + pos: 113, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 8, + column: 0 + }, + frame: ` + 6: }); + 7: + 8: x = 10; + ^ + ` } }; diff --git a/test/function/reassign-import-not-at-top-level-fails/_config.js b/test/function/reassign-import-not-at-top-level-fails/_config.js index b4cba92..43b3a2c 100644 --- a/test/function/reassign-import-not-at-top-level-fails/_config.js +++ b/test/function/reassign-import-not-at-top-level-fails/_config.js @@ -3,10 +3,22 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows assignments to imported bindings not at the top level', - error: function ( err ) { - assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { character: 95, line: 7, column: 2 }); - assert.ok( /Illegal reassignment/.test( err.message ) ); + error: { + code: 'ILLEGAL_REASSIGNMENT', + message: `Illegal reassignment to import 'x'`, + pos: 95, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 7, + column: 2 + }, + frame: ` + 5: } + 6: export function bar () { + 7: x = 1; + ^ + 8: } + ` } }; diff --git a/test/function/reexport-missing-error/_config.js b/test/function/reexport-missing-error/_config.js index 6918683..66bcadc 100644 --- a/test/function/reexport-missing-error/_config.js +++ b/test/function/reexport-missing-error/_config.js @@ -1,8 +1,21 @@ +var path = require( 'path' ); var assert = require( 'assert' ); module.exports = { description: 'reexporting a missing identifier should print an error', - error: function ( error ) { - assert.ok( /^'foo' is not exported/.test( error.message ) ); + error: { + code: 'MISSING_EXPORT', + message: `'foo' is not exported by empty.js`, + pos: 9, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 1, + column: 9 + }, + frame: ` + 1: export { foo as bar } from './empty.js'; + ^ + `, + url: 'https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module' } }; diff --git a/test/function/report-transform-error-file/_config.js b/test/function/report-transform-error-file/_config.js index c856974..56dc1bc 100644 --- a/test/function/report-transform-error-file/_config.js +++ b/test/function/report-transform-error-file/_config.js @@ -1,9 +1,11 @@ +var path = require( 'path' ); var assert = require( 'assert' ); module.exports = { description: 'reports which file caused a transform error', options: { plugins: [{ + name: 'bad-plugin', transform: function ( code, id ) { if ( /foo/.test( id ) ) { throw new Error( 'nope' ); @@ -11,7 +13,10 @@ module.exports = { } }] }, - error: function ( err ) { - assert.ok( ~err.message.indexOf( 'foo.js' ) ); + error: { + code: 'BAD_TRANSFORMER', + message: `Error transforming foo.js with 'bad-plugin' plugin: nope`, + plugin: 'bad-plugin', + id: path.resolve( __dirname, 'foo.js' ) } }; diff --git a/test/function/throws-not-found-module/_config.js b/test/function/throws-not-found-module/_config.js index 04963a3..8aee10d 100644 --- a/test/function/throws-not-found-module/_config.js +++ b/test/function/throws-not-found-module/_config.js @@ -3,7 +3,8 @@ var path = require( 'path' ); module.exports = { description: 'throws error if module is not found', - error: function ( err ) { - assert.equal( err.message, 'Could not resolve \'./mod\' from ' + path.resolve( __dirname, 'main.js' ) ); + error: { + code: 'UNRESOLVED_IMPORT', + message: `Could not resolve './mod' from main.js` } -}; \ No newline at end of file +}; diff --git a/test/function/throws-only-first-transform/_config.js b/test/function/throws-only-first-transform/_config.js index c97907e..4bfd127 100644 --- a/test/function/throws-only-first-transform/_config.js +++ b/test/function/throws-only-first-transform/_config.js @@ -7,23 +7,22 @@ module.exports = { plugins: [ { name: 'plugin1', - transform: function () { - throw Error('Something happend 1'); + transform () { + throw Error( 'Something happened 1' ); } }, { name: 'plugin2', - transform: function () { - throw Error('Something happend 2'); + transform () { + throw Error( 'Something happened 2' ); } } ] }, - error: function ( err ) { - var id = path.resolve( __dirname, 'main.js' ); - assert.equal( err.rollupTransform, true ); - assert.equal( err.id, id ); - assert.equal( err.plugin, 'plugin1' ); - assert.equal( err.message, 'Error transforming ' + id + ' with \'plugin1\' plugin: Something happend 1' ); + error: { + code: 'BAD_TRANSFORMER', + message: `Error transforming main.js with 'plugin1' plugin: Something happened 1`, + plugin: 'plugin1', + id: path.resolve( __dirname, 'main.js' ) } -}; \ No newline at end of file +}; diff --git a/test/function/update-expression-of-import-fails/_config.js b/test/function/update-expression-of-import-fails/_config.js index cf72241..5e813c5 100644 --- a/test/function/update-expression-of-import-fails/_config.js +++ b/test/function/update-expression-of-import-fails/_config.js @@ -3,10 +3,21 @@ var assert = require( 'assert' ); module.exports = { description: 'disallows updates to imported bindings', - error: function ( err ) { - assert.equal( path.normalize( err.file ), path.resolve( __dirname, 'main.js' ) ); - assert.deepEqual( err.loc, { character: 28, line: 3, column: 0 }); - assert.ok( /Illegal reassignment/.test( err.message ) ); + error: { + code: 'ILLEGAL_REASSIGNMENT', + message: `Illegal reassignment to import 'a'`, + pos: 28, + loc: { + file: path.resolve( __dirname, 'main.js' ), + line: 3, + column: 0 + }, + frame: ` + 1: import { a } from './foo'; + 2: + 3: a++; + ^ + ` } }; diff --git a/test/test.js b/test/test.js index 7401b66..cf5348b 100644 --- a/test/test.js +++ b/test/test.js @@ -341,11 +341,7 @@ describe( 'rollup', function () { if ( unintendedError ) throw unintendedError; }, err => { if ( config.error ) { - if ( typeof config.error === 'object' ) { - compareError( err, config.error ); - } else { - config.error( err ); - } + compareError( err, config.error ); } else { throw err; } From 4a2e791ab0e19c2f0a6892bacf379170ed77f789 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Mon, 2 Jan 2017 10:38:02 -0500 Subject: [PATCH 22/28] lint --- src/ast/nodes/CallExpression.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/ast/nodes/CallExpression.js b/src/ast/nodes/CallExpression.js index dc3e6b5..d743838 100644 --- a/src/ast/nodes/CallExpression.js +++ b/src/ast/nodes/CallExpression.js @@ -1,5 +1,3 @@ -import { locate } from 'locate-character'; -import error from '../../utils/error.js'; import Node from '../Node.js'; import isProgramLevel from '../utils/isProgramLevel.js'; import callHasEffects from './shared/callHasEffects.js'; From dc53d0561480bc1ce634570aea7e7e4fa64520d1 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Mon, 2 Jan 2017 11:09:29 -0500 Subject: [PATCH 23/28] use error helpers in more places --- src/utils/getExportMode.js | 11 +++++++++-- src/utils/transformBundle.js | 9 ++++++--- test/function/export-type-mismatch-b/_config.js | 5 +++-- test/function/export-type-mismatch-c/_config.js | 5 +++-- test/function/export-type-mismatch/_config.js | 5 +++-- .../throws-only-first-transform-bundle/_config.js | 13 +++++++------ test/test.js | 2 +- 7 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/utils/getExportMode.js b/src/utils/getExportMode.js index d525550..d61b128 100644 --- a/src/utils/getExportMode.js +++ b/src/utils/getExportMode.js @@ -1,7 +1,11 @@ import { keys } from './object.js'; +import error from './error.js'; function badExports ( option, keys ) { - throw new Error( `'${option}' was specified for options.exports, but entry module has following exports: ${keys.join(', ')}` ); + error({ + code: 'INVALID_EXPORT_OPTION', + message: `'${option}' was specified for options.exports, but entry module has following exports: ${keys.join(', ')}` + }); } export default function getExportMode ( bundle, {exports: exportMode, moduleName, format} ) { @@ -35,7 +39,10 @@ export default function getExportMode ( bundle, {exports: exportMode, moduleName } if ( !/(?:default|named|none)/.test( exportMode ) ) { - throw new Error( `options.exports must be 'default', 'named', 'none', 'auto', or left unspecified (defaults to 'auto')` ); + error({ + code: 'INVALID_EXPORT_OPTION', + message: `options.exports must be 'default', 'named', 'none', 'auto', or left unspecified (defaults to 'auto')` + }); } return exportMode; diff --git a/src/utils/transformBundle.js b/src/utils/transformBundle.js index 8569ad5..03eb966 100644 --- a/src/utils/transformBundle.js +++ b/src/utils/transformBundle.js @@ -1,4 +1,5 @@ import { decode } from 'sourcemap-codec'; +import error from './error.js'; export default function transformBundle ( code, plugins, sourceMapChain, options ) { return plugins.reduce( ( code, plugin ) => { @@ -9,9 +10,11 @@ export default function transformBundle ( code, plugins, sourceMapChain, options try { result = plugin.transformBundle( code, { format : options.format } ); } catch ( err ) { - err.plugin = plugin.name; - err.message = `Error transforming bundle${plugin.name ? ` with '${plugin.name}' plugin` : ''}: ${err.message}`; - throw err; + error({ + code: 'BAD_BUNDLE_TRANSFORMER', + message: `Error transforming bundle${plugin.name ? ` with '${plugin.name}' plugin` : ''}: ${err.message}`, + plugin: plugin.name + }); } if ( result == null ) return code; diff --git a/test/function/export-type-mismatch-b/_config.js b/test/function/export-type-mismatch-b/_config.js index 25a7220..e501330 100644 --- a/test/function/export-type-mismatch-b/_config.js +++ b/test/function/export-type-mismatch-b/_config.js @@ -5,7 +5,8 @@ module.exports = { bundleOptions: { exports: 'blah' }, - generateError: function ( err ) { - assert.ok( /options\.exports must be 'default', 'named', 'none', 'auto', or left unspecified/.test( err.message ) ); + generateError: { + code: 'INVALID_EXPORT_OPTION', + message: `options.exports must be 'default', 'named', 'none', 'auto', or left unspecified (defaults to 'auto')` } }; diff --git a/test/function/export-type-mismatch-c/_config.js b/test/function/export-type-mismatch-c/_config.js index 8df0d58..a1eca46 100644 --- a/test/function/export-type-mismatch-c/_config.js +++ b/test/function/export-type-mismatch-c/_config.js @@ -5,7 +5,8 @@ module.exports = { bundleOptions: { exports: 'none' }, - generateError: function ( err ) { - assert.ok( /'none' was specified for options\.exports/.test( err.message ) ); + generateError: { + code: 'INVALID_EXPORT_OPTION', + message: `'none' was specified for options.exports, but entry module has following exports: default` } }; diff --git a/test/function/export-type-mismatch/_config.js b/test/function/export-type-mismatch/_config.js index 5b321d6..1ff9fd5 100644 --- a/test/function/export-type-mismatch/_config.js +++ b/test/function/export-type-mismatch/_config.js @@ -5,7 +5,8 @@ module.exports = { bundleOptions: { exports: 'default' }, - generateError: function ( err ) { - assert.ok( /'default' was specified for options\.exports/.test( err.message ) ); + generateError: { + code: 'INVALID_EXPORT_OPTION', + message: `'default' was specified for options.exports, but entry module has following exports: foo` } }; diff --git a/test/function/throws-only-first-transform-bundle/_config.js b/test/function/throws-only-first-transform-bundle/_config.js index 9097f52..21804e7 100644 --- a/test/function/throws-only-first-transform-bundle/_config.js +++ b/test/function/throws-only-first-transform-bundle/_config.js @@ -7,19 +7,20 @@ module.exports = { { name: 'plugin1', transformBundle: function () { - throw Error('Something happend 1'); + throw Error( 'Something happened 1' ); } }, { name: 'plugin2', transformBundle: function () { - throw Error('Something happend 2'); + throw Error( 'Something happened 2' ); } } ] }, - generateError: function ( err ) { - assert.equal( err.plugin, 'plugin1' ); - assert.equal( err.message, 'Error transforming bundle with \'plugin1\' plugin: Something happend 1' ); + generateError: { + code: 'BAD_BUNDLE_TRANSFORMER', + plugin: 'plugin1', + message: `Error transforming bundle with 'plugin1' plugin: Something happened 1` } -}; \ No newline at end of file +}; diff --git a/test/test.js b/test/test.js index cf5348b..250b25a 100644 --- a/test/test.js +++ b/test/test.js @@ -276,7 +276,7 @@ describe( 'rollup', function () { } } catch ( err ) { if ( config.generateError ) { - config.generateError( err ); + compareError( err, config.generateError ); } else { unintendedError = err; } From dd9a93f094ca4c1dd2268791b34f1549c5c6beaf Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Mon, 2 Jan 2017 11:30:01 -0500 Subject: [PATCH 24/28] more error stuff --- src/Bundle.js | 7 ++++++- src/Module.js | 16 ++++++++++++---- src/finalisers/iife.js | 6 +++++- src/finalisers/umd.js | 6 +++++- src/rollup.js | 14 +++++++++----- src/utils/collapseSourcemaps.js | 5 ++++- src/utils/defaults.js | 9 ++++++++- 7 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/Bundle.js b/src/Bundle.js index c945184..0cfe895 100644 --- a/src/Bundle.js +++ b/src/Bundle.js @@ -474,7 +474,12 @@ export default class Bundle { const indentString = getIndentString( magicString, options ); const finalise = finalisers[ options.format ]; - if ( !finalise ) throw new Error( `You must specify an output type - valid options are ${keys( finalisers ).join( ', ' )}` ); + if ( !finalise ) { + error({ + code: 'INVALID_OPTION', + message: `You must specify an output type - valid options are ${keys( finalisers ).join( ', ' )}` + }); + } timeStart( 'render format' ); diff --git a/src/Module.js b/src/Module.js index dadd5a5..ce59dee 100644 --- a/src/Module.js +++ b/src/Module.js @@ -121,7 +121,10 @@ export default class Module { const name = specifier.exported.name; if ( this.exports[ name ] || this.reexports[ name ] ) { - throw new Error( `A module cannot have multiple exports with the same name ('${name}')` ); + this.error({ + code: 'DUPLICATE_EXPORT', + message: `A module cannot have multiple exports with the same name ('${name}')` + }, specifier.start ); } this.reexports[ name ] = { @@ -141,8 +144,10 @@ export default class Module { const identifier = ( node.declaration.id && node.declaration.id.name ) || node.declaration.name; if ( this.exports.default ) { - // TODO indicate location - throw new Error( 'A module can only have one default export' ); + this.error({ + code: 'DUPLICATE_EXPORT', + message: `A module can only have one default export` + }, node.start ); } this.exports.default = { @@ -182,7 +187,10 @@ export default class Module { const exportedName = specifier.exported.name; if ( this.exports[ exportedName ] || this.reexports[ exportedName ] ) { - throw new Error( `A module cannot have multiple exports with the same name ('${exportedName}')` ); + this.error({ + code: 'DUPLICATE_EXPORT', + message: `A module cannot have multiple exports with the same name ('${exportedName}')` + }, specifier.start ); } this.exports[ exportedName ] = { localName }; diff --git a/src/finalisers/iife.js b/src/finalisers/iife.js index 2d6121e..aad4491 100644 --- a/src/finalisers/iife.js +++ b/src/finalisers/iife.js @@ -1,5 +1,6 @@ import { blank } from '../utils/object.js'; import { getName } from '../utils/map-helpers.js'; +import error from '../utils/error.js'; import getInteropBlock from './shared/getInteropBlock.js'; import getExportBlock from './shared/getExportBlock.js'; import getGlobalNameMaker from './shared/getGlobalNameMaker.js'; @@ -36,7 +37,10 @@ export default function iife ( bundle, magicString, { exportMode, indentString, const args = bundle.externalModules.map( getName ); if ( exportMode !== 'none' && !name ) { - throw new Error( 'You must supply options.moduleName for IIFE bundles' ); + error({ + code: 'INVALID_OPTION', + message: `You must supply options.moduleName for IIFE bundles` + }); } if ( exportMode === 'named' ) { diff --git a/src/finalisers/umd.js b/src/finalisers/umd.js index 655c77a..b624307 100644 --- a/src/finalisers/umd.js +++ b/src/finalisers/umd.js @@ -1,5 +1,6 @@ import { blank } from '../utils/object.js'; import { getName, quotePath, req } from '../utils/map-helpers.js'; +import error from '../utils/error.js'; import getInteropBlock from './shared/getInteropBlock.js'; import getExportBlock from './shared/getExportBlock.js'; import getGlobalNameMaker from './shared/getGlobalNameMaker.js'; @@ -28,7 +29,10 @@ const wrapperOutro = '\n\n})));'; export default function umd ( bundle, magicString, { exportMode, indentString, intro, outro }, options ) { if ( exportMode !== 'none' && !options.moduleName ) { - throw new Error( 'You must supply options.moduleName for UMD bundles' ); + error({ + code: 'INVALID_OPTION', + message: 'You must supply options.moduleName for UMD bundles' + }); } warnOnBuiltins( bundle ); diff --git a/src/rollup.js b/src/rollup.js index c3e6379..a87fcf8 100644 --- a/src/rollup.js +++ b/src/rollup.js @@ -4,6 +4,7 @@ import { writeFile } from './utils/fs.js'; import { assign, keys } from './utils/object.js'; import { mapSequence } from './utils/promise.js'; import validateKeys from './utils/validateKeys.js'; +import error from './utils/error.js'; import { SOURCEMAPPING_URL } from './utils/sourceMappingURL.js'; import Bundle from './Bundle.js'; @@ -50,15 +51,15 @@ function checkOptions ( options ) { return new Error( 'The `transform`, `load`, `resolveId` and `resolveExternal` options are deprecated in favour of a unified plugin API. See https://github.com/rollup/rollup/wiki/Plugins for details' ); } - const error = validateKeys( keys(options), ALLOWED_KEYS ); - if ( error ) return error; + const err = validateKeys( keys(options), ALLOWED_KEYS ); + if ( err ) return err; return null; } export function rollup ( options ) { - const error = checkOptions ( options ); - if ( error ) return Promise.reject( error ); + const err = checkOptions ( options ); + if ( err ) return Promise.reject( err ); const bundle = new Bundle( options ); @@ -105,7 +106,10 @@ export function rollup ( options ) { generate, write: options => { if ( !options || !options.dest ) { - throw new Error( 'You must supply options.dest to bundle.write' ); + error({ + code: 'MISSING_OPTION', + message: 'You must supply options.dest to bundle.write' + }); } const dest = options.dest; diff --git a/src/utils/collapseSourcemaps.js b/src/utils/collapseSourcemaps.js index 122a148..bd65cf4 100644 --- a/src/utils/collapseSourcemaps.js +++ b/src/utils/collapseSourcemaps.js @@ -1,4 +1,5 @@ import { encode } from 'sourcemap-codec'; +import error from './error.js'; import { dirname, relative, resolve } from './path.js'; class Source { @@ -51,7 +52,9 @@ class Link { } else if ( sourcesContent[ sourceIndex ] == null ) { sourcesContent[ sourceIndex ] = traced.source.content; } else if ( traced.source.content != null && sourcesContent[ sourceIndex ] !== traced.source.content ) { - throw new Error( `Multiple conflicting contents for sourcemap source ${source.filename}` ); + error({ + message: `Multiple conflicting contents for sourcemap source ${source.filename}` + }); } segment[1] = sourceIndex; diff --git a/src/utils/defaults.js b/src/utils/defaults.js index c5dd054..d2757dd 100644 --- a/src/utils/defaults.js +++ b/src/utils/defaults.js @@ -1,6 +1,7 @@ import { lstatSync, readdirSync, readFileSync, realpathSync } from './fs.js'; // eslint-disable-line import { basename, dirname, isAbsolute, resolve } from './path.js'; import { blank } from './object.js'; +import error from './error.js'; export function load ( id ) { return readFileSync( id, 'utf-8' ); @@ -27,7 +28,13 @@ function addJsExtensionIfNecessary ( file ) { } export function resolveId ( importee, importer ) { - if ( typeof process === 'undefined' ) throw new Error( `It looks like you're using Rollup in a non-Node.js environment. This means you must supply a plugin with custom resolveId and load functions. See https://github.com/rollup/rollup/wiki/Plugins for more information` ); + if ( typeof process === 'undefined' ) { + error({ + code: 'MISSING_PROCESS', + message: `It looks like you're using Rollup in a non-Node.js environment. This means you must supply a plugin with custom resolveId and load functions`, + url: 'https://github.com/rollup/rollup/wiki/Plugins' + }); + } // absolute paths are left untouched if ( isAbsolute( importee ) ) return addJsExtensionIfNecessary( resolve( importee ) ); From 35785fff1211791e05bb7f607201ad2f1f8f769d Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 3 Jan 2017 11:04:01 -0500 Subject: [PATCH 25/28] more consistent error logging in CLI --- bin/src/handleError.js | 65 ------------- bin/src/logging.js | 47 +++++++++ bin/src/runRollup.js | 209 ++++++++++++++++++++--------------------- 3 files changed, 151 insertions(+), 170 deletions(-) delete mode 100644 bin/src/handleError.js create mode 100644 bin/src/logging.js diff --git a/bin/src/handleError.js b/bin/src/handleError.js deleted file mode 100644 index 6882108..0000000 --- a/bin/src/handleError.js +++ /dev/null @@ -1,65 +0,0 @@ -import chalk from 'chalk'; - -function stderr ( msg ) { - console.error( msg ); // eslint-disable-line no-console -} - -const handlers = { - MISSING_CONFIG: () => { - stderr( chalk.red( 'Config file must export an options object. See https://github.com/rollup/rollup/wiki/Command-Line-Interface#using-a-config-file' ) ); - }, - - MISSING_EXTERNAL_CONFIG: err => { - stderr( chalk.red( `Could not resolve config file ${err.config}` ) ); - }, - - MISSING_INPUT_OPTION: () => { - stderr( chalk.red( 'You must specify an --input (-i) option' ) ); - }, - - MISSING_OUTPUT_OPTION: () => { - stderr( chalk.red( 'You must specify an --output (-o) option when creating a file with a sourcemap' ) ); - }, - - MISSING_NAME: () => { - stderr( chalk.red( 'You must supply a name for UMD exports (e.g. `--name myModule`)' ) ); - }, - - PARSE_ERROR: err => { - stderr( chalk.red( `Error parsing ${err.file}: ${err.message}` ) ); - }, - - ONE_AT_A_TIME: () => { - stderr( chalk.red( 'rollup can only bundle one file at a time' ) ); - }, - - DUPLICATE_IMPORT_OPTIONS: () => { - stderr( chalk.red( 'use --input, or pass input path as argument' ) ); - }, - - ROLLUP_WATCH_NOT_INSTALLED: () => { - stderr( chalk.red( 'rollup --watch depends on the rollup-watch package, which could not be found. Install it with ' ) + chalk.cyan( 'npm install -D rollup-watch' ) ); - }, - - WATCHER_MISSING_INPUT_OR_OUTPUT: () => { - stderr( chalk.red( 'must specify --input and --output when using rollup --watch' ) ); - } -}; - -export default function handleError ( err, recover ) { - const handler = handlers[ err && err.code ]; - - if ( handler ) { - handler( err ); - } else { - stderr( chalk.red( err.message || err ) ); - - if ( err.stack ) { - stderr( chalk.grey( err.stack ) ); - } - } - - stderr( `Type ${chalk.cyan( 'rollup --help' )} for help, or visit https://github.com/rollup/rollup/wiki` ); - - if ( !recover ) process.exit( 1 ); -} diff --git a/bin/src/logging.js b/bin/src/logging.js new file mode 100644 index 0000000..33b84af --- /dev/null +++ b/bin/src/logging.js @@ -0,0 +1,47 @@ +import chalk from 'chalk'; +import relativeId from '../../src/utils/relativeId.js'; + +if ( !process.stderr.isTTY ) chalk.enabled = false; +const warnSymbol = process.stderr.isTTY ? `⚠️ ` : `Warning: `; +const errorSymbol = process.stderr.isTTY ? `🚨 ` : `Error: `; + +// log to stderr to keep `rollup main.js > bundle.js` from breaking +export const stderr = console.error.bind( console ); // eslint-disable-line no-console + +export function handleWarning ( warning ) { + stderr( `${warnSymbol}${chalk.bold( warning.message )}` ); + + if ( warning.url ) { + stderr( chalk.cyan( warning.url ) ); + } + + if ( warning.loc ) { + stderr( `${relativeId( warning.loc.file )} (${warning.loc.line}:${warning.loc.column})` ); + } + + if ( warning.frame ) { + stderr( chalk.dim( warning.frame ) ); + } + + stderr( '' ); +} + +export function handleError ( err, recover ) { + stderr( `${errorSymbol}${chalk.bold( err.message )}` ); + + if ( err.url ) { + stderr( chalk.cyan( err.url ) ); + } + + if ( err.loc ) { + stderr( `${relativeId( err.loc.file )} (${err.loc.line}:${err.loc.column})` ); + } + + if ( err.frame ) { + stderr( chalk.dim( err.frame ) ); + } + + stderr( '' ); + + if ( !recover ) process.exit( 1 ); +} diff --git a/bin/src/runRollup.js b/bin/src/runRollup.js index 2629d4b..6b0130e 100644 --- a/bin/src/runRollup.js +++ b/bin/src/runRollup.js @@ -2,27 +2,26 @@ import { realpathSync } from 'fs'; import * as rollup from 'rollup'; import relative from 'require-relative'; import chalk from 'chalk'; -import handleError from './handleError'; -import relativeId from '../../src/utils/relativeId.js'; +import { handleWarning, handleError, stderr } from './logging.js'; import SOURCEMAPPING_URL from './sourceMappingUrl.js'; import { install as installSourcemapSupport } from 'source-map-support'; installSourcemapSupport(); -if ( !process.stderr.isTTY ) chalk.enabled = false; -const warnSymbol = process.stderr.isTTY ? `⚠️ ` : `Warning: `; - -// stderr to stderr to keep `rollup main.js > bundle.js` from breaking -const stderr = console.error.bind( console ); // eslint-disable-line no-console - export default function runRollup ( command ) { if ( command._.length > 1 ) { - handleError({ code: 'ONE_AT_A_TIME' }); + handleError({ + code: 'ONE_AT_A_TIME', + message: 'rollup can only bundle one file at a time' + }); } if ( command._.length === 1 ) { if ( command.input ) { - handleError({ code: 'DUPLICATE_IMPORT_OPTIONS' }); + handleError({ + code: 'DUPLICATE_IMPORT_OPTIONS', + message: 'use --input, or pass input path as argument' + }); } command.input = command._[0]; @@ -51,7 +50,10 @@ export default function runRollup ( command ) { config = relative.resolve( pkgName, process.cwd() ); } catch ( err ) { if ( err.code === 'MODULE_NOT_FOUND' ) { - handleError({ code: 'MISSING_EXTERNAL_CONFIG', config }); + handleError({ + code: 'MISSING_EXTERNAL_CONFIG', + message: `Could not resolve config file ${config}` + }); } throw err; @@ -64,37 +66,38 @@ export default function runRollup ( command ) { rollup.rollup({ entry: config, - onwarn: message => { - if ( message.code === 'UNRESOLVED_IMPORT' ) return; - stderr( message.toString() ); + onwarn: warning => { + if ( warning.code === 'UNRESOLVED_IMPORT' ) return; + handleWarning( warning ); } - }).then( bundle => { - const { code } = bundle.generate({ - format: 'cjs' - }); + }) + .then( bundle => { + const { code } = bundle.generate({ + format: 'cjs' + }); - // temporarily override require - const defaultLoader = require.extensions[ '.js' ]; - require.extensions[ '.js' ] = ( m, filename ) => { - if ( filename === config ) { - m._compile( code, filename ); - } else { - defaultLoader( m, filename ); - } - }; + // temporarily override require + const defaultLoader = require.extensions[ '.js' ]; + require.extensions[ '.js' ] = ( m, filename ) => { + if ( filename === config ) { + m._compile( code, filename ); + } else { + defaultLoader( m, filename ); + } + }; - try { const options = require( config ); if ( Object.keys( options ).length === 0 ) { - handleError({ code: 'MISSING_CONFIG' }); + handleError({ + code: 'MISSING_CONFIG', + message: 'Config file must export an options object', + url: 'https://github.com/rollup/rollup/wiki/Command-Line-Interface#using-a-config-file' + }); } execute( options, command ); require.extensions[ '.js' ] = defaultLoader; - } catch ( err ) { - handleError( err ); - } - }) - .catch( stderr ); + }) + .catch( handleError ); } else { execute( {}, command ); } @@ -157,21 +160,7 @@ function execute ( options, command ) { if ( seen.has( str ) ) return; seen.add( str ); - stderr( `${warnSymbol}${chalk.bold( warning.message )}` ); - - if ( warning.url ) { - stderr( chalk.cyan( warning.url ) ); - } - - if ( warning.loc ) { - stderr( `${relativeId( warning.loc.file )} (${warning.loc.line}:${warning.loc.column})` ); - } - - if ( warning.frame ) { - stderr( chalk.dim( warning.frame ) ); - } - - stderr( '' ); + handleWarning( warning ); }; } @@ -184,50 +173,52 @@ function execute ( options, command ) { } }); - try { - if ( command.watch ) { - if ( !options.entry || ( !options.dest && !options.targets ) ) { - handleError({ code: 'WATCHER_MISSING_INPUT_OR_OUTPUT' }); - } + if ( command.watch ) { + if ( !options.entry || ( !options.dest && !options.targets ) ) { + handleError({ + code: 'WATCHER_MISSING_INPUT_OR_OUTPUT', + message: 'must specify --input and --output when using rollup --watch' + }); + } - try { - const watch = relative( 'rollup-watch', process.cwd() ); - const watcher = watch( rollup, options ); + try { + const watch = relative( 'rollup-watch', process.cwd() ); + const watcher = watch( rollup, options ); - watcher.on( 'event', event => { - switch ( event.code ) { - case 'STARTING': // TODO this isn't emitted by newer versions of rollup-watch - stderr( 'checking rollup-watch version...' ); - break; + watcher.on( 'event', event => { + switch ( event.code ) { + case 'STARTING': // TODO this isn't emitted by newer versions of rollup-watch + stderr( 'checking rollup-watch version...' ); + break; - case 'BUILD_START': - stderr( 'bundling...' ); - break; + case 'BUILD_START': + stderr( 'bundling...' ); + break; - case 'BUILD_END': - stderr( 'bundled in ' + event.duration + 'ms. Watching for changes...' ); - break; + case 'BUILD_END': + stderr( 'bundled in ' + event.duration + 'ms. Watching for changes...' ); + break; - case 'ERROR': - handleError( event.error, true ); - break; + case 'ERROR': + handleError( event.error, true ); + break; - default: - stderr( 'unknown event', event ); - } - }); - } catch ( err ) { - if ( err.code === 'MODULE_NOT_FOUND' ) { - err.code = 'ROLLUP_WATCH_NOT_INSTALLED'; + default: + stderr( 'unknown event', event ); } - - handleError( err ); + }); + } catch ( err ) { + if ( err.code === 'MODULE_NOT_FOUND' ) { + handleError({ + code: 'ROLLUP_WATCH_NOT_INSTALLED', + message: 'rollup --watch depends on the rollup-watch package, which could not be found. Install it with npm install -D rollup-watch' + }); } - } else { - bundle( options ).catch( handleError ); + + handleError( err ); } - } catch ( err ) { - handleError( err ); + } else { + bundle( options ).catch( handleError ); } } @@ -244,34 +235,42 @@ function assign ( target, source ) { function bundle ( options ) { if ( !options.entry ) { - handleError({ code: 'MISSING_INPUT_OPTION' }); + handleError({ + code: 'MISSING_INPUT_OPTION', + message: 'You must specify an --input (-i) option' + }); } - return rollup.rollup( options ).then( bundle => { - if ( options.dest ) { - return bundle.write( options ); - } + return rollup.rollup( options ) + .then( bundle => { + if ( options.dest ) { + return bundle.write( options ); + } - if ( options.targets ) { - let result = null; + if ( options.targets ) { + let result = null; - options.targets.forEach( target => { - result = bundle.write( assign( clone( options ), target ) ); - }); + options.targets.forEach( target => { + result = bundle.write( assign( clone( options ), target ) ); + }); - return result; - } + return result; + } - if ( options.sourceMap && options.sourceMap !== 'inline' ) { - handleError({ code: 'MISSING_OUTPUT_OPTION' }); - } + if ( options.sourceMap && options.sourceMap !== 'inline' ) { + handleError({ + code: 'MISSING_OUTPUT_OPTION', + message: 'You must specify an --output (-o) option when creating a file with a sourcemap' + }); + } - let { code, map } = bundle.generate( options ); + let { code, map } = bundle.generate( options ); - if ( options.sourceMap === 'inline' ) { - code += `\n//# ${SOURCEMAPPING_URL}=${map.toUrl()}\n`; - } + if ( options.sourceMap === 'inline' ) { + code += `\n//# ${SOURCEMAPPING_URL}=${map.toUrl()}\n`; + } - process.stdout.write( code ); - }); + process.stdout.write( code ); + }) + .catch( handleError ); } From 5dfae02e87cd93c84b616663c21bacb585a0c1b1 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Tue, 3 Jan 2017 12:06:39 -0500 Subject: [PATCH 26/28] -> v0.40.0 --- CHANGELOG.md | 5 +++++ package.json | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06b8d67..ff7b231 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # rollup changelog +## 0.40.0 + +* [BREAKING] Better, more consistent error logging ([#1212](https://github.com/rollup/rollup/pull/1212)) +* Don't use colours and emojis for non-TTY stderr ([#1201](https://github.com/rollup/rollup/issues/1201)) + ## 0.39.2 * Prevent mutation of cached ASTs (fixes stack overflow with rollup-watch) ([#1205](https://github.com/rollup/rollup/pull/1205)) diff --git a/package.json b/package.json index b7a285d..0ae3125 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollup", - "version": "0.39.2", + "version": "0.40.0", "description": "Next-generation ES6 module bundler", "main": "dist/rollup.js", "module": "dist/rollup.es.js", From e7fa75cfe4946d988675b3545c49335920a5cba5 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 4 Jan 2017 10:38:07 -0500 Subject: [PATCH 27/28] handle export default{} --- src/ast/nodes/ExportDefaultDeclaration.js | 2 +- test/function/export-default-no-space/_config.js | 8 ++++++++ test/function/export-default-no-space/main.js | 1 + 3 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 test/function/export-default-no-space/_config.js create mode 100644 test/function/export-default-no-space/main.js diff --git a/src/ast/nodes/ExportDefaultDeclaration.js b/src/ast/nodes/ExportDefaultDeclaration.js index 7027291..2f505b1 100644 --- a/src/ast/nodes/ExportDefaultDeclaration.js +++ b/src/ast/nodes/ExportDefaultDeclaration.js @@ -53,7 +53,7 @@ export default class ExportDefaultDeclaration extends Node { let declaration_start; if ( this.declaration ) { const statementStr = code.original.slice( this.start, this.end ); - declaration_start = this.start + statementStr.match(/^\s*export\s+default\s+/)[0].length; + declaration_start = this.start + statementStr.match(/^\s*export\s+default\s*/)[0].length; } if ( this.shouldInclude || this.declaration.activated ) { diff --git a/test/function/export-default-no-space/_config.js b/test/function/export-default-no-space/_config.js new file mode 100644 index 0000000..8a27d49 --- /dev/null +++ b/test/function/export-default-no-space/_config.js @@ -0,0 +1,8 @@ +const assert = require( 'assert' ); + +module.exports = { + description: 'handles default exports with no space before declaration', + exports: exports => { + assert.deepEqual( exports, {} ); + } +}; diff --git a/test/function/export-default-no-space/main.js b/test/function/export-default-no-space/main.js new file mode 100644 index 0000000..f9749a6 --- /dev/null +++ b/test/function/export-default-no-space/main.js @@ -0,0 +1 @@ +export default{}; From 6cf998a6f74e731a0bb1dad9da9387ff30c74487 Mon Sep 17 00:00:00 2001 From: Rich-Harris Date: Thu, 5 Jan 2017 09:24:19 -0500 Subject: [PATCH 28/28] -> v0.40.1 --- CHANGELOG.md | 4 ++++ package.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff7b231..92fa262 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # rollup changelog +## 0.40.1 + +* Allow missing space between `export default` and declaration ([#1218](https://github.com/rollup/rollup/pull/1218)) + ## 0.40.0 * [BREAKING] Better, more consistent error logging ([#1212](https://github.com/rollup/rollup/pull/1212)) diff --git a/package.json b/package.json index 0ae3125..2ca30a2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "rollup", - "version": "0.40.0", + "version": "0.40.1", "description": "Next-generation ES6 module bundler", "main": "dist/rollup.js", "module": "dist/rollup.es.js",