use error helpers consistently, update tests

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',

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;

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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