Merge pull request #14118 from webpack/bugfix/multiple-module-externals

fix null module externals
webpack · Sep 3, 2021 · a513b13 · a513b13
2 parents 825bbc1 + b50d033
commit a513b13
Show file tree

Hide file tree

Showing 12 changed files with 134 additions and 42 deletions.
diff --git a/lib/ExternalModule.js b/lib/ExternalModule.js
@@ -13,6 +13,7 @@ const Module = require("./Module");
 const RuntimeGlobals = require("./RuntimeGlobals");
 const Template = require("./Template");
 const StaticExportsDependency = require("./dependencies/StaticExportsDependency");
+const createHash = require("./util/createHash");
 const extractUrlAndGlobal = require("./util/extractUrlAndGlobal");
 const makeSerializable = require("./util/makeSerializable");
 const propertyAccess = require("./util/propertyAccess");
@@ -159,18 +160,25 @@ const getSourceForImportExternal = (moduleAndSpecifiers, runtimeTemplate) => {
 };
 
 class ModuleExternalInitFragment extends InitFragment {
-	constructor(id, request) {
-		const identifier = `__WEBPACK_EXTERNAL_MODULE_${Template.toIdentifier(
-			`${id}`
-		)}__`;
+	constructor(request, ident) {
+		if (ident === undefined) {
+			ident = Template.toIdentifier(request);
+			if (ident !== request) {
+				ident += `_${createHash("md4")
+					.update(request)
+					.digest("hex")
+					.slice(0, 8)}`;
+			}
+		}
+		const identifier = `__WEBPACK_EXTERNAL_MODULE_${ident}__`;
 		super(
 			`import * as ${identifier} from ${JSON.stringify(request)};\n`,
 			InitFragment.STAGE_HARMONY_IMPORTS,
 			0,
-			`external module import ${id}`
+			`external module import ${ident}`
 		);
+		this._ident = ident;
 		this._identifier = identifier;
-		this._id = id;
 		this._request = request;
 	}
 
@@ -185,8 +193,8 @@ register(
 	"ModuleExternalInitFragment",
 	{
 		serialize(obj, { write }) {
-			write(obj._id);
 			write(obj._request);
+			write(obj._ident);
 		},
 		deserialize({ read }) {
 			return new ModuleExternalInitFragment(read(), read());
@@ -236,10 +244,7 @@ const getSourceForModuleExternal = (
 ) => {
 	if (!Array.isArray(moduleAndSpecifiers))
 		moduleAndSpecifiers = [moduleAndSpecifiers];
-	const initFragment = new ModuleExternalInitFragment(
-		id,
-		moduleAndSpecifiers[0]
-	);
+	const initFragment = new ModuleExternalInitFragment(moduleAndSpecifiers[0]);
 	const baseAccess = `${initFragment.getNamespaceIdentifier()}${propertyAccess(
 		moduleAndSpecifiers,
 		1
@@ -400,7 +405,7 @@ class ExternalModule extends Module {
 	 * @returns {string} a unique identifier of the module
 	 */
 	identifier() {
-		return "external " + JSON.stringify(this.request);
+		return `external ${this.externalType} ${JSON.stringify(this.request)}`;
 	}
 
 	/**
@@ -531,18 +536,20 @@ class ExternalModule extends Module {
 			case "umd":
 			case "umd2":
 			case "system":
-			case "jsonp":
+			case "jsonp": {
+				const id = chunkGraph.getModuleId(this);
 				return getSourceForAmdOrUmdExternal(
-					chunkGraph.getModuleId(this),
+					id !== null ? id : this.identifier(),
 					this.isOptional(moduleGraph),
 					request,
 					runtimeTemplate
 				);
+			}
 			case "import":
 				return getSourceForImportExternal(request, runtimeTemplate);
 			case "script":
 				return getSourceForScriptExternal(request, runtimeTemplate);
-			case "module":
+			case "module": {
 				if (!this.buildInfo.module) {
 					if (!runtimeTemplate.supportsDynamicImport()) {
 						throw new Error(
@@ -559,12 +566,14 @@ class ExternalModule extends Module {
 						"The target environment doesn't support EcmaScriptModule syntax so it's not possible to use external type 'module'"
 					);
 				}
+				const id = chunkGraph.getModuleId(this);
 				return getSourceForModuleExternal(
-					chunkGraph.getModuleId(this),
+					id !== null ? id : this.identifier(),
 					request,
 					moduleGraph.getExportsInfo(this),
 					runtime
 				);
+			}
 			case "var":
 			case "promise":
 			case "const":

diff --git a/lib/InitFragment.js b/lib/InitFragment.js
@@ -6,6 +6,7 @@
 "use strict";
 
 const { ConcatSource } = require("webpack-sources");
+const makeSerializable = require("./util/makeSerializable");
 
 /** @typedef {import("webpack-sources").Source} Source */
 /** @typedef {import("./Generator").GenerateContext} GenerateContext */
@@ -123,8 +124,30 @@ class InitFragment {
 			return source;
 		}
 	}
+
+	serialize(context) {
+		const { write } = context;
+
+		write(this.content);
+		write(this.stage);
+		write(this.position);
+		write(this.key);
+		write(this.endContent);
+	}
+
+	deserialize(context) {
+		const { read } = context;
+
+		this.content = read();
+		this.stage = read();
+		this.position = read();
+		this.key = read();
+		this.endContent = read();
+	}
 }
 
+makeSerializable(InitFragment, "webpack/lib/InitFragment");
+
 InitFragment.prototype.merge = undefined;
 
 InitFragment.STAGE_CONSTANTS = 10;

diff --git a/lib/optimize/MangleExportsPlugin.js b/lib/optimize/MangleExportsPlugin.js
@@ -39,13 +39,28 @@ const comparator = compareSelect(e => e.name, compareStringsNumeric);
 /**
  * @param {boolean} deterministic use deterministic names
  * @param {ExportsInfo} exportsInfo exports info
+ * @param {boolean} isNamespace is namespace object
  * @returns {void}
  */
-const mangleExportsInfo = (deterministic, exportsInfo) => {
+const mangleExportsInfo = (deterministic, exportsInfo, isNamespace) => {
 	if (!canMangle(exportsInfo)) return;
 	const usedNames = new Set();
 	/** @type {ExportInfo[]} */
 	const mangleableExports = [];
+
+	// Avoid to renamed exports that are not provided when
+	// 1. it's not a namespace export: non-provided exports can be found in prototype chain
+	// 2. there are other provided exports and deterministic mode is chosen:
+	//    non-provided exports would break the determinism
+	let avoidMangleNonProvided = !isNamespace;
+	if (!avoidMangleNonProvided && deterministic) {
+		for (const exportInfo of exportsInfo.ownedExports) {
+			if (exportInfo.provided !== false) {
+				avoidMangleNonProvided = true;
+				break;
+			}
+		}
+	}
 	for (const exportInfo of exportsInfo.ownedExports) {
 		const name = exportInfo.name;
 		if (!exportInfo.hasUsedName()) {
@@ -59,7 +74,7 @@ const mangleExportsInfo = (deterministic, exportsInfo) => {
 					name.length === 2 &&
 					/^[a-zA-Z_$][a-zA-Z0-9_$]|^[1-9][0-9]/.test(name)) ||
 				// Don't rename exports that are not provided
-				exportInfo.provided !== true
+				(avoidMangleNonProvided && exportInfo.provided !== true)
 			) {
 				exportInfo.setUsedName(name);
 				usedNames.add(name);
@@ -73,7 +88,7 @@ const mangleExportsInfo = (deterministic, exportsInfo) => {
 				used === UsageState.OnlyPropertiesUsed ||
 				used === UsageState.Unused
 			) {
-				mangleExportsInfo(deterministic, exportInfo.exportsInfo);
+				mangleExportsInfo(deterministic, exportInfo.exportsInfo, false);
 			}
 		}
 	}
@@ -143,8 +158,10 @@ class MangleExportsPlugin {
 				"MangleExportsPlugin",
 				modules => {
 					for (const module of modules) {
+						const isNamespace =
+							module.buildMeta && module.buildMeta.exportsType === "namespace";
 						const exportsInfo = moduleGraph.getExportsInfo(module);
-						mangleExportsInfo(deterministic, exportsInfo);
+						mangleExportsInfo(deterministic, exportsInfo, isNamespace);
 					}
 				}
 			);

diff --git a/lib/util/internalSerializables.js b/lib/util/internalSerializables.js
@@ -164,6 +164,7 @@ module.exports = {
 	DllModule: () => require("../DllModule"),
 	ExternalModule: () => require("../ExternalModule"),
 	FileSystemInfo: () => require("../FileSystemInfo"),
+	InitFragment: () => require("../InitFragment"),
 	InvalidDependenciesModuleWarning: () =>
 		require("../InvalidDependenciesModuleWarning"),
 	Module: () => require("../Module"),

diff --git a/test/ConfigTestCases.template.js b/test/ConfigTestCases.template.js
@@ -486,7 +486,7 @@ const describeCases = config => {
 																		"unlinked",
 																		referencingModule
 																	),
-																	module.context,
+																	referencingModule.context,
 																	true
 																);
 															}

diff --git a/test/__snapshots__/ConfigCacheTestCases.longtest.js.snap b/test/__snapshots__/ConfigCacheTestCases.longtest.js.snap
@@ -29,14 +29,14 @@ exports[`ConfigCacheTestCases records issue-2991 exported tests should write rel
   \\"modules\\": {
     \\"byIdentifier\\": {
       \\"./test.js\\": 393,
-      \\"external \\\\\\"fs\\\\\\"\\": 747,
-      \\"external \\\\\\"path\\\\\\"\\": 622,
+      \\"external node-commonjs \\\\\\"fs\\\\\\"\\": 147,
+      \\"external node-commonjs \\\\\\"path\\\\\\"\\": 17,
       \\"ignored|./.|pkgs/somepackage/foo\\": 802
     },
     \\"usedIds\\": [
+      17,
+      147,
       393,
-      622,
-      747,
       802
     ]
   }
@@ -62,16 +62,16 @@ exports[`ConfigCacheTestCases records issue-7339 exported tests should write rel
       \\"./dependencies/foo.js\\": 117,
       \\"./dependencies|sync|/^\\\\\\\\.\\\\\\\\/.*$/\\": 412,
       \\"./test.js\\": 393,
-      \\"external \\\\\\"fs\\\\\\"\\": 747,
-      \\"external \\\\\\"path\\\\\\"\\": 622
+      \\"external node-commonjs \\\\\\"fs\\\\\\"\\": 147,
+      \\"external node-commonjs \\\\\\"path\\\\\\"\\": 17
     },
     \\"usedIds\\": [
+      17,
       117,
+      147,
       379,
       393,
-      412,
-      622,
-      747
+      412
     ]
   }
 }"

diff --git a/test/__snapshots__/ConfigTestCases.basictest.js.snap b/test/__snapshots__/ConfigTestCases.basictest.js.snap
@@ -29,14 +29,14 @@ exports[`ConfigTestCases records issue-2991 exported tests should write relative
   \\"modules\\": {
     \\"byIdentifier\\": {
       \\"./test.js\\": 393,
-      \\"external \\\\\\"fs\\\\\\"\\": 747,
-      \\"external \\\\\\"path\\\\\\"\\": 622,
+      \\"external node-commonjs \\\\\\"fs\\\\\\"\\": 147,
+      \\"external node-commonjs \\\\\\"path\\\\\\"\\": 17,
       \\"ignored|./.|pkgs/somepackage/foo\\": 802
     },
     \\"usedIds\\": [
+      17,
+      147,
       393,
-      622,
-      747,
       802
     ]
   }
@@ -62,16 +62,16 @@ exports[`ConfigTestCases records issue-7339 exported tests should write relative
       \\"./dependencies/foo.js\\": 117,
       \\"./dependencies|sync|/^\\\\\\\\.\\\\\\\\/.*$/\\": 412,
       \\"./test.js\\": 393,
-      \\"external \\\\\\"fs\\\\\\"\\": 747,
-      \\"external \\\\\\"path\\\\\\"\\": 622
+      \\"external node-commonjs \\\\\\"fs\\\\\\"\\": 147,
+      \\"external node-commonjs \\\\\\"path\\\\\\"\\": 17
     },
     \\"usedIds\\": [
+      17,
       117,
+      147,
       379,
       393,
-      412,
-      622,
-      747
+      412
     ]
   }
 }"

diff --git a/test/configCases/externals/concatenated-module/index.js b/test/configCases/externals/concatenated-module/index.js
@@ -0,0 +1,12 @@
+import fs1 from "fs";
+import fs2 from "module-fs";
+import fsPromises1 from "fs-promises";
+import fsPromises2 from "module-fs-promises";
+import path1 from "path";
+import path2 from "module-path";
+
+it("should be possible to import multiple module externals", () => {
+	expect(fs2).toBe(fs1);
+	expect(path2).toBe(path1);
+	expect(fsPromises2).toBe(fsPromises1);
+});
diff --git a/test/configCases/externals/concatenated-module/test.filter.js b/test/configCases/externals/concatenated-module/test.filter.js
@@ -0,0 +1,3 @@
+module.exports = () => {
+	return !process.version.startsWith("v10.");
+};
diff --git a/test/configCases/externals/concatenated-module/webpack.config.js b/test/configCases/externals/concatenated-module/webpack.config.js
@@ -0,0 +1,25 @@
+/** @type {(variant: boolean) => import("../../../../").Configuration} */
+const config = o => ({
+	externals: {
+		"module-fs": o ? "module fs" : "module fs/promises",
+		fs: o ? "node-commonjs fs" : "node-commonjs fs/promises",
+		"module-fs-promises": o ? ["module fs", "promises"] : "module fs/promises",
+		"fs-promises": o
+			? ["node-commonjs fs", "promises"]
+			: "node-commonjs fs/promises",
+		"module-path": "module path",
+		path: "node-commonjs path"
+	},
+	optimization: {
+		concatenateModules: true,
+		usedExports: true,
+		providedExports: true,
+		mangleExports: true
+	},
+	target: "node14",
+	experiments: {
+		outputModule: true
+	}
+});
+
+module.exports = [config(false), config(true)];
diff --git a/test/helpers/asModule.js b/test/helpers/asModule.js
@@ -14,9 +14,9 @@ module.exports = async (something, context, unlinked) => {
 	const code = [...new Set(["default", ...Object.keys(something)])]
 		.map(
 			name =>
-				`const _${name} = ${SYNTHETIC_MODULES_STORE}[${i}][${JSON.stringify(
-					name
-				)}]; export { _${name} as ${name}};`
+				`const _${name} = ${SYNTHETIC_MODULES_STORE}[${i}]${
+					name === "default" ? "" : `[${JSON.stringify(name)}]`
+				}; export { _${name} as ${name}};`
 		)
 		.join("\n");
 	const m = new vm.SourceTextModule(code, {

diff --git a/types.d.ts b/types.d.ts
@@ -4512,6 +4512,8 @@ declare abstract class InitFragment<Context> {
 	endContent?: string | Source;
 	getContent(context: Context): string | Source;
 	getEndContent(context: Context): undefined | string | Source;
+	serialize(context?: any): void;
+	deserialize(context?: any): void;
 	merge: any;
 }
 declare interface InputFileSystem {