add module.error helper for consistently reporting errors (#545)

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;

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;

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 ) {

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();
+			   ^
+		`
 	}
 };

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();
+			   ^
+		`
 	}
 };

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';
+			   ^
+		`
 	}
 };

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)'
 	}
 };

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'
 	}
 };

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`
 	}
 };

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;
+			          ^
+		`
 	}
 };

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 };
+			            ^
+		`
 	}
 };

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';
+			            ^
+		`
 	}
 };

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: }
+		`
 	}
 };

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: }
+		`
 	}
 };

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';
 }

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`
 	}
 };

test/function/reports-syntax-error-locations/_config.js

@@ -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 ) );
-	}
-};

test/function/reports-syntax-error-locations/main.js

@@ -1 +0,0 @@
-var 42 = answer;

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 ) {
+							if ( typeof config.error === 'object' ) {
+								compareError( err, config.error );
+							} else {
 								config.error( err );
+							}
 						} else {
 							throw err;
 						}