Merge pull request #6542 from mc-zone/feature/module-build-error-with…

…-loader-name

Add loader name to error message. Resolves #2878

lib/ModuleBuildError.js

@@ -8,12 +8,17 @@ const WebpackError = require("./WebpackError");
 const { cutOffLoaderExecution } = require("./ErrorHelpers");
 
 class ModuleBuildError extends WebpackError {
-	constructor(module, err) {
-		let message = "Module build failed: ";
+	constructor(module, err, { from = null } = {}) {
+		let message = "Module build failed";
 		let details = undefined;
+		if (from) {
+			message += ` (from ${from}):\n`;
+		} else {
+			message += ": ";
+		}
 		if (err !== null && typeof err === "object") {
 			if (typeof err.stack === "string" && err.stack) {
-				var stack = cutOffLoaderExecution(err.stack);
+				const stack = cutOffLoaderExecution(err.stack);
 				if (!err.hideStack) {
 					message += stack;
 				} else {
@@ -29,6 +34,8 @@ class ModuleBuildError extends WebpackError {
 			} else {
 				message += err;
 			}
+		} else {
+			message = err;
 		}
 
 		super(message);

lib/ModuleError.js

@@ -8,9 +8,19 @@ const WebpackError = require("./WebpackError");
 const { cleanUp } = require("./ErrorHelpers");
 
 class ModuleError extends WebpackError {
-	constructor(module, err) {
-		super(err && typeof err === "object" && err.message ? err.message : err);
-
+	constructor(module, err, { from = null } = {}) {
+		let message = "Module Error";
+		if (from) {
+			message += ` (from ${from}):\n`;
+		} else {
+			message += ": ";
+		}
+		if (err && typeof err === "object" && err.message) {
+			message += err.message;
+		} else if (err) {
+			message += err;
+		}
+		super(message);
 		this.name = "ModuleError";
 		this.module = module;
 		this.error = err;

lib/ModuleWarning.js

@@ -8,13 +8,19 @@ const WebpackError = require("./WebpackError");
 const { cleanUp } = require("./ErrorHelpers");
 
 class ModuleWarning extends WebpackError {
-	constructor(module, warning) {
-		super(
-			warning && typeof warning === "object" && warning.message
-				? warning.message
-				: warning
-		);
-
+	constructor(module, warning, { from = null } = {}) {
+		let message = "Module Warning";
+		if (from) {
+			message += ` (from ${from}):\n`;
+		} else {
+			message += ": ";
+		}
+		if (warning && typeof warning === "object" && warning.message) {
+			message += warning.message;
+		} else if (warning) {
+			message += warning;
+		}
+		super(message);
 		this.name = "ModuleWarning";
 		this.module = module;
 		this.warning = warning;

lib/NormalModule.js

@@ -166,19 +166,30 @@ class NormalModule extends Module {
 	}
 
 	createLoaderContext(resolver, options, compilation, fs) {
+		const requestShortener = compilation.runtimeTemplate.requestShortener;
 		const loaderContext = {
 			version: 2,
 			emitWarning: warning => {
 				if (!(warning instanceof Error)) {
 					warning = new NonErrorEmittedError(warning);
 				}
-				this.warnings.push(new ModuleWarning(this, warning));
+				const currentLoader = this.getCurrentLoader(loaderContext);
+				this.warnings.push(
+					new ModuleWarning(this, warning, {
+						from: requestShortener.shorten(currentLoader.loader)
+					})
+				);
 			},
 			emitError: error => {
 				if (!(error instanceof Error)) {
 					error = new NonErrorEmittedError(error);
 				}
-				this.errors.push(new ModuleError(this, error));
+				const currentLoader = this.getCurrentLoader(loaderContext);
+				this.errors.push(
+					new ModuleError(this, error, {
+						from: requestShortener.shorten(currentLoader.loader)
+					})
+				);
 			},
 			exec: (code, filename) => {
 				// @ts-ignore Argument of type 'this' is not assignable to parameter of type 'Module'.
@@ -219,6 +230,19 @@ class NormalModule extends Module {
 		return loaderContext;
 	}
 
+	getCurrentLoader(loaderContext, index = loaderContext.loaderIndex) {
+		if (
+			this.loaders &&
+			this.loaders.length &&
+			index < this.loaders.length &&
+			index >= 0 &&
+			this.loaders[index]
+		) {
+			return this.loaders[index];
+		}
+		return null;
+	}
+
 	createSource(source, resourceBuffer, sourceMap) {
 		// if there is no identifier return raw source
 		if (!this.identifier) {
@@ -272,7 +296,17 @@ class NormalModule extends Module {
 				}
 
 				if (err) {
-					const error = new ModuleBuildError(this, err);
+					if (!(err instanceof Error)) {
+						err = new NonErrorEmittedError(err);
+					}
+					const currentLoader = this.getCurrentLoader(loaderContext);
+					const error = new ModuleBuildError(this, err, {
+						from:
+							currentLoader &&
+							compilation.runtimeTemplate.requestShortener.shorten(
+								currentLoader.loader
+							)
+					});
 					return callback(error);
 				}
 
@@ -282,10 +316,17 @@ class NormalModule extends Module {
 				const extraInfo = result.result.length >= 2 ? result.result[2] : null;
 
 				if (!Buffer.isBuffer(source) && typeof source !== "string") {
-					const error = new ModuleBuildError(
-						this,
-						new Error("Final loader didn't return a Buffer or String")
+					const currentLoader = this.getCurrentLoader(loaderContext, 0);
+					const err = new Error(
+						`Final loader (${
+							currentLoader
+								? compilation.runtimeTemplate.requestShortener.shorten(
+										currentLoader.loader
+								  )
+								: "unknown"
+						}) didn't return a Buffer or String`
 					);
+					const error = new ModuleBuildError(this, err);
 					return callback(error);
 				}
 

test/Errors.test.js

@@ -43,6 +43,15 @@ describe("Errors", () => {
 			callback(stats.errors, stats.warnings);
 		});
 	}
+
+	function getErrorsPromise(options, callback) {
+		return new Promise((resolve, reject) => {
+			getErrors(options, (errors, warnings) => {
+				callback(errors, warnings);
+				resolve();
+			});
+		});
+	}
 	it("should throw an error if file doesn't exist", done => {
 		getErrors(
 			{
@@ -190,4 +199,162 @@ describe("Errors", () => {
 			}
 		);
 	});
+	it("should show loader name when emit/throw errors or warnings from loaders", () => {
+		return Promise.all([
+			getErrorsPromise(
+				{
+					mode: "development",
+					entry: "./entry-point-error-loader-required.js"
+				},
+				(errors, warnings) => {
+					expect(warnings).toHaveLength(1);
+					expect(warnings[0].split("\n")[1]).toMatch(
+						/^Module Warning \(from .\/emit-error-loader.js\):$/
+					);
+					expect(errors).toHaveLength(1);
+					expect(errors[0].split("\n")[1]).toMatch(
+						/^Module Error \(from .\/emit-error-loader.js\):$/
+					);
+				}
+			),
+			getErrorsPromise(
+				{
+					mode: "development",
+					entry: path.resolve(base, "./emit-error-loader") + "!./entry-point.js"
+				},
+				(errors, warnings) => {
+					expect(warnings).toHaveLength(1);
+					expect(warnings[0].split("\n")[1]).toMatch(
+						/^Module Warning \(from .\/emit-error-loader.js\):$/
+					);
+					expect(errors).toHaveLength(1);
+					expect(errors[0].split("\n")[1]).toMatch(
+						/^Module Error \(from .\/emit-error-loader.js\):$/
+					);
+				}
+			),
+			getErrorsPromise(
+				{
+					mode: "development",
+					entry: "./not-a-json.js",
+					module: {
+						rules: [
+							{
+								test: /not-a-json\.js$/,
+								use: [
+									"json-loader",
+									{
+										loader: path.resolve(base, "./emit-error-loader")
+									}
+								]
+							}
+						]
+					}
+				},
+				(errors, warnings) => {
+					expect(warnings).toHaveLength(1);
+					expect(warnings[0].split("\n")[1]).toMatch(
+						/^Module Warning \(from .\/emit-error-loader.js\):$/
+					);
+					expect(errors).toHaveLength(2);
+					expect(errors[0].split("\n")[1]).toMatch(
+						/^Module Error \(from .\/emit-error-loader.js\):$/
+					);
+					expect(errors[1].split("\n")[1]).toMatch(
+						/^Module build failed \(from \(webpack\)\/node_modules\/json-loader\/index.js\):$/
+					);
+				}
+			),
+			getErrorsPromise(
+				{
+					mode: "development",
+					entry: "./entry-point.js",
+					module: {
+						rules: [
+							{
+								test: /entry-point\.js$/,
+								use: path.resolve(base, "./async-error-loader")
+							}
+						]
+					}
+				},
+				(errors, warnings) => {
+					expect(errors).toHaveLength(1);
+					expect(errors[0].split("\n")[1]).toMatch(
+						/^Module build failed \(from .\/async-error-loader.js\):$/
+					);
+				}
+			),
+			getErrorsPromise(
+				{
+					mode: "development",
+					entry: "./entry-point.js",
+					module: {
+						rules: [
+							{
+								test: /entry-point\.js$/,
+								use: path.resolve(base, "./throw-error-loader")
+							}
+						]
+					}
+				},
+				(errors, warnings) => {
+					expect(errors).toHaveLength(1);
+					expect(errors[0].split("\n")[1]).toMatch(
+						/^Module build failed \(from .\/throw-error-loader.js\):$/
+					);
+				}
+			),
+			getErrorsPromise(
+				{
+					mode: "development",
+					entry: "./entry-point.js",
+					module: {
+						rules: [
+							{
+								test: /entry-point\.js$/,
+								use: path.resolve(base, "./irregular-error-loader")
+							}
+						]
+					}
+				},
+				(errors, warnings) => {
+					expect(warnings).toHaveLength(2);
+					expect(warnings[0].split("\n")[1]).toMatch(
+						/^Module Warning \(from .\/irregular-error-loader.js\):$/
+					);
+					expect(warnings[1].split("\n")[1]).toMatch(
+						/^Module Warning \(from .\/irregular-error-loader.js\):$/
+					);
+
+					expect(errors).toHaveLength(3);
+					expect(errors[0].split("\n")[1]).toMatch(
+						/^Module Error \(from .\/irregular-error-loader.js\):$/
+					);
+					expect(errors[1].split("\n")[1]).toMatch(
+						/^Module Error \(from .\/irregular-error-loader.js\):$/
+					);
+					expect(errors[2].split("\n")[1]).toMatch(
+						/^Module build failed \(from .\/irregular-error-loader.js\):$/
+					);
+				}
+			)
+		]);
+	});
+	it("should throw a build error if no source be returned after run loaders", done => {
+		getErrors(
+			{
+				mode: "development",
+				entry: path.resolve(base, "./no-return-loader") + "!./entry-point.js"
+			},
+			(errors, warnings) => {
+				expect(errors).toHaveLength(1);
+				const messages = errors[0].split("\n");
+				expect(messages[1]).toMatch(
+					/^Module build failed: Error: Final loader \(.+\) didn't return a Buffer or String/
+				);
+				done();
+			}
+		);
+	});
 });

test/cases/compile/error-hide-stack/errors.js

@@ -1,3 +1,3 @@
 module.exports = [
-	[/Module build failed: Message\nStack/]
-];
+	[/Module build failed( \(from [^)]+\))?:\nMessage\nStack/]
+];

test/cases/loaders/no-string/errors.js

@@ -0,0 +1,10 @@
+module.exports = [
+	[
+		/\.\/loaders\/no-string\/file\.js \(\.\/loaders\/no-string\/loader\.js!\.\/loaders\/no-string\/file\.js\)/,
+		/Module build failed: Error: Final loader \(\.\/loaders\/no-string\/loader\.js\) didn't return a Buffer or String/
+	],
+	[
+		/\.\/loaders\/no-string\/file\.js \(\.\/loaders\/no-string\/loader\.js!\.\/loaders\/no-string\/pitch-loader\.js!\.\/loaders\/no-string\/file\.js\)/,
+		/Module build failed: Error: Final loader \(\.\/loaders\/no-string\/loader\.js\) didn't return a Buffer or String/
+	]
+];

test/cases/loaders/no-string/file.js

@@ -0,0 +1 @@
+123

test/cases/loaders/no-string/index.js

@@ -0,0 +1,8 @@
+it("should emit the correct error for loaders not returning buffer or string", function() {
+	expect(() => require("./loader.js!./file.js")).toThrowError(
+		/Module build failed/
+	);
+	expect(() => require("./loader.js!./pitch-loader.js!./file.js")).toThrowError(
+		/Module build failed/
+	);
+});

test/cases/loaders/no-string/loader.js

@@ -0,0 +1,3 @@
+module.exports = () => {
+	return 123;
+};

test/cases/loaders/no-string/pitch-loader.js

@@ -0,0 +1,3 @@
+module.exports = () => {
+	return 123;
+};

test/fixtures/errors/async-error-loader.js

@@ -0,0 +1,5 @@
+module.exports = function(source) {
+	const callback = this.async();
+	const error = new Error("this is a callback error");
+	callback(error, source);
+};