Repository: webpack/webpack
Pull request #7672 (2018-07-06 10:57:40)

Refactor HMR hooks
Refactor HMR plugin to change how hooks are installed.

**What kind of change does this PR introduce?**

refactoring

**Did you add tests for your changes?**

already tested

**Does this PR introduce a breaking change?**

yes

**What needs to be documented once your changes are merged?**

Currently HMR hooks are not documented. If some day they are, then it must uses `HotModuleReplacementPlugin.hooks(parser)`.

webpack-bot: <!-- identifier: admin-actions -->

*For maintainers only:*

* [ ] <!-- document --> This need to be documented (issue in webpack/webpack.js.org will be filed when merged)
ooflorent: @jantimon Maybe you want to look at this PR!
jantimon: @ooflorent thanks :) actually I really like the `WeakMap` approach.
It feels cleaner than modifying the webpack internal hooks directly.  
Unfortunately it would be a huge breaking change for my plugin.
ooflorent: For webpack also. That's why we change this for the next major (v5).
webpack-bot: <!-- identifier: ci-result -->

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.
jantimon: I am a huge fan of semantic versioning however releasing a new major often breaks up with the current eco system.

For the new version of the html-webpack-plugin I had to change the hooks anyway so I'll also switch to a `WeakMap` and export the content of https://github.com/jantimon/html-webpack-plugin/blob/webpack-4/lib/hooks.js  as a static helper 👍

Thanks for sharing!! :)
webpack-bot: <!-- identifier: review-outdated -->

@ooflorent Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

Patch:
diff --git a/lib/BannerPlugin.js b/lib/BannerPlugin.js
index 3cd9cee78..1e9aa343c 100644
--- a/lib/BannerPlugin.js
+++ b/lib/BannerPlugin.js
@@ -23,7 +23,13 @@ const wrapComment = str => {
 };
 
 class BannerPlugin {
-	constructor(options = {}) {
+	constructor(options) {
+		if (arguments.length > 1) {
+			throw new Error(
+				"BannerPlugin only takes one argument (pass an options object)"
+			);
+		}
+
 		validateOptions(schema, options, "Banner Plugin");
 
 		if (typeof options === "string" || typeof options === "function") {
@@ -32,7 +38,7 @@ class BannerPlugin {
 			};
 		}
 
-		this.options = options;
+		this.options = options || {};
 
 		if (typeof options.banner === "function") {
 			const getBanner = this.options.banner;
diff --git a/lib/Compilation.js b/lib/Compilation.js
index 7abfe9997..ebc3b56ac 100644
--- a/lib/Compilation.js
+++ b/lib/Compilation.js
@@ -1439,6 +1439,8 @@ class Compilation {
 	 * @returns {DependencyReference} a reference for the dependency
 	 */
 	getDependencyReference(module, dependency) {
+		// TODO remove dep.getReference existance check in webpack 5
+		if (typeof dependency.getReference !== "function") return null;
 		const ref = dependency.getReference();
 		if (!ref) return null;
 		return this.hooks.dependencyReference.call(ref, dependency, module);
diff --git a/lib/EvalSourceMapDevToolPlugin.js b/lib/EvalSourceMapDevToolPlugin.js
index f168f866c..daf9aaddf 100644
--- a/lib/EvalSourceMapDevToolPlugin.js
+++ b/lib/EvalSourceMapDevToolPlugin.js
@@ -7,25 +7,22 @@
 const EvalSourceMapDevToolModuleTemplatePlugin = require("./EvalSourceMapDevToolModuleTemplatePlugin");
 const SourceMapDevToolModuleOptionsPlugin = require("./SourceMapDevToolModuleOptionsPlugin");
 
-/** @typedef {import("./Compiler")} Compiler */
-
 class EvalSourceMapDevToolPlugin {
-	/**
-	 * @param {TODO} options Options object
-	 */
-	constructor(options = {}) {
+	constructor(options) {
+		if (arguments.length > 1) {
+			throw new Error(
+				"EvalSourceMapDevToolPlugin only takes one argument (pass an options object)"
+			);
+		}
 		if (typeof options === "string") {
 			options = {
 				append: options
 			};
 		}
+		if (!options) options = {};
 		this.options = options;
 	}
 
-	/**
-	 * @param {Compiler} compiler Webpack Compiler
-	 * @returns {void}
-	 */
 	apply(compiler) {
 		const options = this.options;
 		compiler.hooks.compilation.tap(
diff --git a/lib/HotModuleReplacementPlugin.js b/lib/HotModuleReplacementPlugin.js
index ad9f25a29..69580c873 100644
--- a/lib/HotModuleReplacementPlugin.js
+++ b/lib/HotModuleReplacementPlugin.js
@@ -11,6 +11,7 @@ const ModuleHotAcceptDependency = require("./dependencies/ModuleHotAcceptDepende
 const ModuleHotDeclineDependency = require("./dependencies/ModuleHotDeclineDependency");
 const ConstDependency = require("./dependencies/ConstDependency");
 const NullFactory = require("./NullFactory");
+const JavascriptParser = require("./JavascriptParser");
 const {
 	evaluateToIdentifier,
 	evaluateToString,
@@ -18,7 +19,30 @@ const {
 	toConstantDependencyWithWebpackRequire
 } = require("./JavascriptParserHelpers");
 
+const hotInitCode = Template.getFunctionContent(
+	require("./HotModuleReplacement.runtime.js")
+);
+
+const parserHooksMap = new WeakMap();
+
 module.exports = class HotModuleReplacementPlugin {
+	static getHooks(parser) {
+		if (!(parser instanceof JavascriptParser)) {
+			throw new TypeError(
+				"The 'parser' argument must be an instance of JavascriptParser"
+			);
+		}
+		let hooks = parserHooksMap.get(parser);
+		if (hooks === undefined) {
+			hooks = {
+				hotAcceptCallback: new SyncBailHook(["expression", "requests"]),
+				hotAcceptWithoutCallback: new SyncBailHook(["expression", "requests"])
+			};
+			parserHooksMap.set(parser, hooks);
+		}
+		return hooks;
+	}
+
 	constructor(options) {
 		this.options = options || {};
 		this.multiStep = this.options.multiStep;
@@ -42,6 +66,11 @@ module.exports = class HotModuleReplacementPlugin {
 		);
 
 		const addParserPlugins = (parser, parserOptions) => {
+			const {
+				hotAcceptCallback,
+				hotAcceptWithoutCallback
+			} = HotModuleReplacementPlugin.getHooks(parser);
+
 			parser.hooks.expression
 				.for("__webpack_hash__")
 				.tap(
@@ -66,19 +95,6 @@ module.exports = class HotModuleReplacementPlugin {
 					)(expr);
 				}
 			);
-			// TODO webpack 5: refactor this, no custom hooks
-			if (!parser.hooks.hotAcceptCallback) {
-				parser.hooks.hotAcceptCallback = new SyncBailHook([
-					"expression",
-					"requests"
-				]);
-			}
-			if (!parser.hooks.hotAcceptWithoutCallback) {
-				parser.hooks.hotAcceptWithoutCallback = new SyncBailHook([
-					"expression",
-					"requests"
-				]);
-			}
 			parser.hooks.call
 				.for("module.hot.accept")
 				.tap("HotModuleReplacementPlugin", expr => {
@@ -105,14 +121,11 @@ module.exports = class HotModuleReplacementPlugin {
 								requests.push(request);
 							});
 							if (expr.arguments.length > 1) {
-								parser.hooks.hotAcceptCallback.call(
-									expr.arguments[1],
-									requests
-								);
+								hotAcceptCallback.call(expr.arguments[1], requests);
 								parser.walkExpression(expr.arguments[1]); // other args are ignored
 								return true;
 							} else {
-								parser.hooks.hotAcceptWithoutCallback.call(expr, requests);
+								hotAcceptWithoutCallback.call(expr, requests);
 								return true;
 							}
 						}
@@ -407,7 +420,3 @@ module.exports = class HotModuleReplacementPlugin {
 		);
 	}
 };
-
-const hotInitCode = Template.getFunctionContent(
-	require("./HotModuleReplacement.runtime.js")
-);
diff --git a/lib/SourceMapDevToolPlugin.js b/lib/SourceMapDevToolPlugin.js
index 12b7499b9..111d7a877 100644
--- a/lib/SourceMapDevToolPlugin.js
+++ b/lib/SourceMapDevToolPlugin.js
@@ -13,6 +13,11 @@ const createHash = require("./util/createHash");
 const validateOptions = require("schema-utils");
 const schema = require("../schemas/plugins/SourceMapDevToolPlugin.json");
 
+const basename = name => {
+	if (!name.includes("/")) return name;
+	return name.substr(name.lastIndexOf("/") + 1);
+};
+
 const assetsCache = new WeakMap();
 
 const getTaskForFile = (file, chunk, options, compilation) => {
@@ -47,9 +52,16 @@ const getTaskForFile = (file, chunk, options, compilation) => {
 };
 
 class SourceMapDevToolPlugin {
-	constructor(options = {}) {
-		validateOptions(schema, options, "SourceMap DevTool Plugin");
+	constructor(options) {
+		if (arguments.length > 1) {
+			throw new Error(
+				"SourceMapDevToolPlugin only takes one argument (pass an options object)"
+			);
+		}
+
+		validateOptions(schema, options || {}, "SourceMap DevTool Plugin");
 
+		if (!options) options = {};
 		this.sourceMapFilename = options.filename;
 		this.sourceMappingURLComment =
 			options.append === false
@@ -240,7 +252,7 @@ class SourceMapDevToolPlugin {
 									? path.relative(options.fileContext, filename)
 									: filename,
 								query,
-								basename: path.basename(filename),
+								basename: basename(filename),
 								contentHash: createHash("md4")
 									.update(sourceMapString)
 									.digest("hex")
diff --git a/lib/dependencies/HarmonyImportDependencyParserPlugin.js b/lib/dependencies/HarmonyImportDependencyParserPlugin.js
index 7f9fcde20..ef64157cb 100644
--- a/lib/dependencies/HarmonyImportDependencyParserPlugin.js
+++ b/lib/dependencies/HarmonyImportDependencyParserPlugin.js
@@ -4,7 +4,7 @@
 */
 "use strict";
 
-const { SyncBailHook } = require("tapable");
+const HotModuleReplacementPlugin = require("../HotModuleReplacementPlugin");
 const HarmonyImportSideEffectDependency = require("./HarmonyImportSideEffectDependency");
 const HarmonyImportSpecifierDependency = require("./HarmonyImportSpecifierDependency");
 const HarmonyAcceptImportDependency = require("./HarmonyAcceptImportDependency");
@@ -152,20 +152,11 @@ module.exports = class HarmonyImportDependencyParserPlugin {
 				if (args) parser.walkExpressions(args);
 				return true;
 			});
-		// TODO webpack 5: refactor this, no custom hooks
-		if (!parser.hooks.hotAcceptCallback) {
-			parser.hooks.hotAcceptCallback = new SyncBailHook([
-				"expression",
-				"requests"
-			]);
-		}
-		if (!parser.hooks.hotAcceptWithoutCallback) {
-			parser.hooks.hotAcceptWithoutCallback = new SyncBailHook([
-				"expression",
-				"requests"
-			]);
-		}
-		parser.hooks.hotAcceptCallback.tap(
+		const {
+			hotAcceptCallback,
+			hotAcceptWithoutCallback
+		} = HotModuleReplacementPlugin.getHooks(parser);
+		hotAcceptCallback.tap(
 			"HarmonyImportDependencyParserPlugin",
 			(expr, requests) => {
 				const harmonyParserScope = parser.state.harmonyParserScope;
@@ -194,7 +185,7 @@ module.exports = class HarmonyImportDependencyParserPlugin {
 				}
 			}
 		);
-		parser.hooks.hotAcceptWithoutCallback.tap(
+		hotAcceptWithoutCallback.tap(
 			"HarmonyImportDependencyParserPlugin",
 			(expr, requests) => {
 				const dependencies = requests.map(request => {
diff --git a/lib/web/WebEnvironmentPlugin.js b/lib/web/WebEnvironmentPlugin.js
new file mode 100644
index 000000000..9b0df5746
--- /dev/null
+++ b/lib/web/WebEnvironmentPlugin.js
@@ -0,0 +1,18 @@
+/*
+	MIT License http://www.opensource.org/licenses/mit-license.php
+	Author Tobias Koppers @sokra
+*/
+"use strict";
+
+class WebEnvironmentPlugin {
+	constructor(inputFileSystem, outputFileSystem) {
+		this.inputFileSystem = inputFileSystem;
+		this.outputFileSystem = outputFileSystem;
+	}
+
+	apply(compiler) {
+		compiler.outputFileSystem = this.outputFileSystem;
+	}
+}
+
+module.exports = WebEnvironmentPlugin;
diff --git a/lib/webpack.web.js b/lib/webpack.web.js
new file mode 100644
index 000000000..1f60eab7f
--- /dev/null
+++ b/lib/webpack.web.js
@@ -0,0 +1,31 @@
+/*
+	MIT License http://www.opensource.org/licenses/mit-license.php
+	Author Tobias Koppers @sokra
+*/
+"use strict";
+
+const Compiler = require("./Compiler");
+const WebEnvironmentPlugin = require("./web/WebEnvironmentPlugin");
+const WebpackOptionsApply = require("./WebpackOptionsApply");
+const WebpackOptionsDefaulter = require("./WebpackOptionsDefaulter");
+
+const webpack = (options, callback) => {
+	new WebpackOptionsDefaulter().process(options);
+
+	const compiler = new Compiler();
+	compiler.options = new WebpackOptionsApply().process(options, compiler);
+	new WebEnvironmentPlugin(
+		options.inputFileSystem,
+		options.outputFileSystem
+	).apply(compiler);
+	if (callback) {
+		compiler.run(callback);
+	}
+	return compiler;
+};
+module.exports = webpack;
+
+webpack.WebpackOptionsDefaulter = WebpackOptionsDefaulter;
+webpack.WebpackOptionsApply = WebpackOptionsApply;
+webpack.Compiler = Compiler;
+webpack.WebEnvironmentPlugin = WebEnvironmentPlugin;
diff --git a/package.json b/package.json
index 96d46dc80..69bb0a38d 100644
--- a/package.json
+++ b/package.json
@@ -90,6 +90,7 @@
   },
   "homepage": "https://github.com/webpack/webpack",
   "main": "lib/webpack.js",
+  "web": "lib/webpack.web.js",
   "bin": "./bin/webpack.js",
   "files": [
     "lib/",


Test patch:
diff --git a/test/WebEnvironmentPlugin.unittest.js b/test/WebEnvironmentPlugin.unittest.js
new file mode 100644
index 000000000..137956a00
--- /dev/null
+++ b/test/WebEnvironmentPlugin.unittest.js
@@ -0,0 +1,23 @@
+"use strict";
+
+const WebEnvironmentPlugin = require("../lib/web/WebEnvironmentPlugin");
+
+describe("WebEnvironmentPlugin", () => {
+	describe("apply", () => {
+		const WebEnvironmentPluginInstance = new WebEnvironmentPlugin(
+			"inputFileSystem",
+			"outputFileSystem"
+		);
+		const compileSpy = {
+			outputFileSystem: "otherOutputFileSystem"
+		};
+
+		WebEnvironmentPluginInstance.apply(compileSpy);
+
+		it("should set compiler.outputFileSystem information with the same as set in WebEnvironmentPlugin", () => {
+			expect(compileSpy.outputFileSystem).toBe(
+				WebEnvironmentPluginInstance.outputFileSystem
+			);
+		});
+	});
+});