Preserve context for amd/cjs dynamic non-arrow requires

Resolves #3092
rollup · Sep 7, 2021 · 431bd28 · 431bd28
1 parent 9d3b104
commit 431bd28
Show file tree

Hide file tree

Showing 33 changed files with 534 additions and 87 deletions.
diff --git a/src/ast/nodes/ImportExpression.ts b/src/ast/nodes/ImportExpression.ts
@@ -126,8 +126,19 @@ export default class ImportExpression extends NodeBase {
 	private getDynamicImportMechanismAndHelper(
 		resolution: Module | ExternalModule | string | null,
 		exportMode: 'none' | 'named' | 'default' | 'external',
-		{ compact, dynamicImportFunction, format, interop }: NormalizedOutputOptions,
-		{ _, getDirectReturnFunctionLeft, directReturnFunctionRight }: GenerateCodeSnippets,
+		{
+			compact,
+			dynamicImportFunction,
+			format,
+			generatedCode: { arrowFunctions },
+			interop
+		}: NormalizedOutputOptions,
+		{
+			_,
+			directReturnFunctionRight,
+			getDirectReturnFunctionLeft,
+			getDirectReturnIifeLeft
+		}: GenerateCodeSnippets,
 		pluginDriver: PluginDriver
 	): { helper: string | null; mechanism: DynamicImportMechanism | null } {
 		const mechanism = pluginDriver.hookFirstSync('renderDynamicImport', [
@@ -142,24 +153,31 @@ export default class ImportExpression extends NodeBase {
 		if (mechanism) {
 			return { helper: null, mechanism };
 		}
+		const hasDynamicTarget = !this.resolution || typeof this.resolution === 'string';
 		switch (format) {
 			case 'cjs': {
-				const leftStart = `Promise.resolve().then(${getDirectReturnFunctionLeft([], {
+				const helper = getInteropHelper(resolution, exportMode, interop);
+				let left = `require(`;
+				let right = `)`;
+				if (helper) {
+					left = `/*#__PURE__*/${helper}(${left}`;
+					right += ')';
+				}
+				left = `Promise.resolve().then(${getDirectReturnFunctionLeft([], {
 					functionReturn: true,
 					name: null
-				})}`;
-				const helper = getInteropHelper(resolution, exportMode, interop);
+				})}${left}`;
+				right += `${directReturnFunctionRight})`;
+				if (!arrowFunctions && hasDynamicTarget) {
+					left = getDirectReturnIifeLeft(['t'], `${left}t${right}`, {
+						needsArrowReturnParens: false,
+						needsWrappedFunction: true
+					});
+					right = ')';
+				}
 				return {
 					helper,
-					mechanism: helper
-						? {
-								left: `${leftStart}/*#__PURE__*/${helper}(require(`,
-								right: `))${directReturnFunctionRight})`
-						  }
-						: {
-								left: `${leftStart}require(`,
-								right: `)${directReturnFunctionRight})`
-						  }
+					mechanism: { left, right }
 				};
 			}
 			case 'amd': {
@@ -172,15 +190,21 @@ export default class ImportExpression extends NodeBase {
 							name: null
 					  })}${resolve}(/*#__PURE__*/${helper}(m))${directReturnFunctionRight}`
 					: resolve;
+				let left = `new Promise(${getDirectReturnFunctionLeft([resolve, reject], {
+					functionReturn: false,
+					name: null
+				})}require([`;
+				let right = `],${_}${resolveNamespace},${_}${reject})${directReturnFunctionRight})`;
+				if (!arrowFunctions && hasDynamicTarget) {
+					left = getDirectReturnIifeLeft(['t'], `${left}t${right}`, {
+						needsArrowReturnParens: false,
+						needsWrappedFunction: true
+					});
+					right = ')';
+				}
 				return {
 					helper,
-					mechanism: {
-						left: `new Promise(${getDirectReturnFunctionLeft([resolve, reject], {
-							functionReturn: false,
-							name: null
-						})}require([`,
-						right: `],${_}${resolveNamespace},${_}${reject})${directReturnFunctionRight})`
-					}
+					mechanism: { left, right }
 				};
 			}
 			case 'system':
@@ -225,9 +249,3 @@ const accessedImportGlobals: Record<string, string[]> = {
 	cjs: ['require'],
 	system: ['module']
 };
-
-// TODO Lukas consider fixing context issue for non-arrow resolutions IF it is dynamic:
-// import(this.foo) ->
-// (function (arg) {
-//   return Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require(arg))
-// })} (this.foo))
diff --git a/src/utils/generateCodeSnippets.ts b/src/utils/generateCodeSnippets.ts
@@ -1,4 +1,3 @@
-import MagicString from 'magic-string';
 import { NormalizedOutputOptions } from '../rollup/types';
 import { RESERVED_NAMES } from './reservedNames';
 
@@ -12,23 +11,20 @@ export interface GenerateCodeSnippets {
 		params: string[],
 		options: { functionReturn: boolean; name: string | null }
 	): string;
-	getFunctionIntro(params: string[], options: { isAsync: boolean; name: string | null }): string;
-	getObject(
-		fields: [key: string | null, value: string][],
-		options: { indent: string; lineBreaks: boolean }
-	): string;
-	getPropertyAccess(name: string): string;
-	renderDirectReturnIife(
+	getDirectReturnIifeLeft(
 		params: string[],
 		returned: string,
-		code: MagicString,
-		argStart: number,
-		argEnd: number,
 		options: {
 			needsArrowReturnParens: boolean | undefined;
 			needsWrappedFunction: boolean | undefined;
 		}
-	): void;
+	): string;
+	getFunctionIntro(params: string[], options: { isAsync: boolean; name: string | null }): string;
+	getObject(
+		fields: [key: string | null, value: string][],
+		options: { indent: string; lineBreaks: boolean }
+	): string;
+	getPropertyAccess(name: string): string;
 }
 
 export function getGenerateCodeSnippets({
@@ -70,6 +66,17 @@ export function getGenerateCodeSnippets({
 		_,
 		directReturnFunctionRight,
 		getDirectReturnFunctionLeft,
+		getDirectReturnIifeLeft: (params, returned, { needsArrowReturnParens, needsWrappedFunction }) =>
+			`${wrapIfNeeded(
+				`${getDirectReturnFunctionLeft(params, {
+					functionReturn: true,
+					name: null
+				})}${wrapIfNeeded(
+					returned,
+					arrowFunctions && needsArrowReturnParens
+				)}${directReturnFunctionRight}`,
+				arrowFunctions || needsWrappedFunction
+			)}(`,
 		getFunctionIntro,
 		getObject(fields, { indent, lineBreaks }) {
 			const prefix = `${lineBreaks ? n : ''}${indent}`;
@@ -87,29 +94,6 @@ export function getGenerateCodeSnippets({
 			isValidPropName(name) ? `.${name}` : `[${JSON.stringify(name)}]`,
 		n,
 		namedDirectReturnFunctionRight: `${directReturnFunctionRight}${arrowFunctions ? ';' : ''}`,
-		renderDirectReturnIife: (
-			params,
-			returned,
-			code,
-			argStart,
-			argEnd,
-			{ needsArrowReturnParens, needsWrappedFunction }
-		) => {
-			code.prependRight(
-				argStart,
-				`${wrapIfNeeded(
-					`${getDirectReturnFunctionLeft(params, {
-						functionReturn: true,
-						name: null
-					})}${wrapIfNeeded(
-						returned,
-						arrowFunctions && needsArrowReturnParens
-					)}${directReturnFunctionRight}`,
-					arrowFunctions || needsWrappedFunction
-				)}(`
-			);
-			code.appendLeft(argEnd, ')');
-		},
 		s
 	};
 }

diff --git a/src/utils/systemJsRendering.ts b/src/utils/systemJsRendering.ts
@@ -48,15 +48,16 @@ export function renderSystemExportFunction(
 	code: MagicString,
 	options: RenderOptions
 ): void {
-	const { _, renderDirectReturnIife } = options.snippets;
-	renderDirectReturnIife(
-		['v'],
-		`${getSystemExportStatement(exportedVariables, options)},${_}v`,
-		code,
+	const { _, getDirectReturnIifeLeft } = options.snippets;
+	code.prependRight(
 		expressionStart,
-		expressionEnd,
-		{ needsArrowReturnParens: true, needsWrappedFunction: needsParens }
+		getDirectReturnIifeLeft(
+			['v'],
+			`${getSystemExportStatement(exportedVariables, options)},${_}v`,
+			{ needsArrowReturnParens: true, needsWrappedFunction: needsParens }
+		)
 	);
+	code.appendLeft(expressionEnd, ')');
 }
 
 export function renderSystemExportSequenceAfterExpression(

diff --git a/test/chunking-form/samples/dynamic-import-dynamic/_expected/amd/main.js b/test/chunking-form/samples/dynamic-import-dynamic/_expected/amd/main.js
@@ -20,6 +20,6 @@ define(['require'], (function (require) { 'use strict';
 
 	var dep = 'dep';
 
-	new Promise(function (resolve, reject) { require([dep], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); });
+	(function (t) { return new Promise(function (resolve, reject) { require([t], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); }); })(dep);
 
 }));
diff --git a/test/chunking-form/samples/dynamic-import-dynamic/_expected/cjs/main.js b/test/chunking-form/samples/dynamic-import-dynamic/_expected/cjs/main.js
@@ -20,4 +20,4 @@ function _interopNamespace(e) {
 
 var dep = 'dep';
 
-Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require(dep)); });
+(function (t) { return Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require(t)); }); })(dep);
diff --git a/test/chunking-form/samples/resolve-dynamic-import/_expected/amd/main.js b/test/chunking-form/samples/resolve-dynamic-import/_expected/amd/main.js
@@ -40,9 +40,9 @@ define(['require', './direct-relative-external', 'to-indirect-relative-external'
 	new Promise(function (resolve, reject) { require(['direct-absolute-external'], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); });
 	new Promise(function (resolve, reject) { require(['to-indirect-absolute-external'], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); });
 
-	new Promise(function (resolve, reject) { require(['dynamic-direct-external' + unknown], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); });
+	(function (t) { return new Promise(function (resolve, reject) { require([t], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); }); })('dynamic-direct-external' + unknown);
 	new Promise(function (resolve, reject) { require(['to-dynamic-indirect-external'], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); });
 	Promise.resolve().then(function () { return existing; });
-	new Promise(function (resolve, reject) { require(['my' + 'replacement'], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); });
+	(function (t) { return new Promise(function (resolve, reject) { require([t], function (m) { resolve(/*#__PURE__*/_interopNamespace(m)); }, reject); }); })('my' + 'replacement');
 
 }));
diff --git a/test/chunking-form/samples/resolve-dynamic-import/_expected/cjs/main.js b/test/chunking-form/samples/resolve-dynamic-import/_expected/cjs/main.js
@@ -45,7 +45,7 @@ Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(requi
 Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require('direct-absolute-external')); });
 Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require('to-indirect-absolute-external')); });
 
-Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require('dynamic-direct-external' + unknown)); });
+(function (t) { return Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require(t)); }); })('dynamic-direct-external' + unknown);
 Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require('to-dynamic-indirect-external')); });
 Promise.resolve().then(function () { return existing; });
-Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require('my' + 'replacement')); });
+(function (t) { return Promise.resolve().then(function () { return /*#__PURE__*/_interopNamespace(require(t)); }); })('my' + 'replacement');
diff --git a/test/form/samples/dynamic-import-this-arrow/_config.js b/test/form/samples/dynamic-import-this-arrow/_config.js
@@ -0,0 +1,12 @@
+const assert = require('assert');
+
+module.exports = {
+	description: 'uses correct "this" in dynamic imports when using arrow functions',
+	options: {
+		external: ['input', 'output'],
+		output: {
+			generatedCode: { arrowFunctions: true },
+			name: 'bundle'
+		}
+	}
+};
diff --git a/test/form/samples/dynamic-import-this-arrow/_expected/amd.js b/test/form/samples/dynamic-import-this-arrow/_expected/amd.js
@@ -0,0 +1,37 @@
+define(['require', 'exports', 'input'], ((require, exports, input) => { 'use strict';
+
+	function _interopNamespace(e) {
+		if (e && e.__esModule) return e;
+		var n = Object.create(null);
+		if (e) {
+			Object.keys(e).forEach(k => {
+				if (k !== 'default') {
+					var d = Object.getOwnPropertyDescriptor(e, k);
+					Object.defineProperty(n, k, d.get ? d : {
+						enumerable: true,
+						get: () => e[k]
+					});
+				}
+			});
+		}
+		n["default"] = e;
+		return Object.freeze(n);
+	}
+
+	class Importer {
+		constructor() {
+			this.outputPath = input.outputPath;
+		}
+
+		getImport() {
+			return new Promise((resolve, reject) => require([this.outputPath], m => resolve(/*#__PURE__*/_interopNamespace(m)), reject));
+		}
+	}
+
+	const promise = new Importer().getImport();
+
+	exports.promise = promise;
+
+	Object.defineProperty(exports, '__esModule', { value: true });
+
+}));
diff --git a/test/form/samples/dynamic-import-this-arrow/_expected/cjs.js b/test/form/samples/dynamic-import-this-arrow/_expected/cjs.js
@@ -0,0 +1,37 @@
+'use strict';
+
+Object.defineProperty(exports, '__esModule', { value: true });
+
+var input = require('input');
+
+function _interopNamespace(e) {
+	if (e && e.__esModule) return e;
+	var n = Object.create(null);
+	if (e) {
+		Object.keys(e).forEach(k => {
+			if (k !== 'default') {
+				var d = Object.getOwnPropertyDescriptor(e, k);
+				Object.defineProperty(n, k, d.get ? d : {
+					enumerable: true,
+					get: () => e[k]
+				});
+			}
+		});
+	}
+	n["default"] = e;
+	return Object.freeze(n);
+}
+
+class Importer {
+	constructor() {
+		this.outputPath = input.outputPath;
+	}
+
+	getImport() {
+		return Promise.resolve().then(() => /*#__PURE__*/_interopNamespace(require(this.outputPath)));
+	}
+}
+
+const promise = new Importer().getImport();
+
+exports.promise = promise;
diff --git a/test/form/samples/dynamic-import-this-arrow/_expected/es.js b/test/form/samples/dynamic-import-this-arrow/_expected/es.js
@@ -0,0 +1,15 @@
+import { outputPath } from 'input';
+
+class Importer {
+	constructor() {
+		this.outputPath = outputPath;
+	}
+
+	getImport() {
+		return import(this.outputPath);
+	}
+}
+
+const promise = new Importer().getImport();
+
+export { promise };
diff --git a/test/form/samples/dynamic-import-this-arrow/_expected/iife.js b/test/form/samples/dynamic-import-this-arrow/_expected/iife.js
@@ -0,0 +1,22 @@
+var bundle = ((exports, input) => {
+	'use strict';
+
+	class Importer {
+		constructor() {
+			this.outputPath = input.outputPath;
+		}
+
+		getImport() {
+			return import(this.outputPath);
+		}
+	}
+
+	const promise = new Importer().getImport();
+
+	exports.promise = promise;
+
+	Object.defineProperty(exports, '__esModule', { value: true });
+
+	return exports;
+
+})({}, input);