Fixed imports/exports that are illegal identifiers in the es output (#…

…4939) * Fixed exports that are illegal identifiers in the es output * Unify shared~ regex into `isValidIdentifier` util * fixed more cases --------- Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>
rollup · Apr 18, 2023 · 3bbf6ae · 3bbf6ae
1 parent f1538ce
commit 3bbf6ae
Show file tree

Hide file tree

Showing 12 changed files with 243 additions and 11 deletions.
diff --git a/src/finalisers/es.ts b/src/finalisers/es.ts
@@ -3,8 +3,12 @@ import type { ChunkDependency, ChunkExports, ImportSpecifier, ReexportSpecifier
 import type { NormalizedOutputOptions } from '../rollup/types';
 import type { GenerateCodeSnippets } from '../utils/generateCodeSnippets';
 import { getHelpersBlock } from '../utils/interopHelpers';
+import { isValidIdentifier } from '../utils/isValidIdentifier';
 import type { FinaliserOptions } from './index';
 
+const safeExportName = (name: string): string =>
+	isValidIdentifier(name) ? name : JSON.stringify(name);
+
 export default function es(
 	magicString: MagicStringBundle,
 	{ accessedGlobals, indent: t, intro, outro, dependencies, exports, snippets }: FinaliserOptions,
@@ -68,7 +72,7 @@ function getImportBlock(
 						.map(specifier =>
 							specifier.imported === specifier.local
 								? specifier.imported
-								: `${specifier.imported} as ${specifier.local}`
+								: `${safeExportName(specifier.imported)} as ${specifier.local}`
 						)
 						.join(`,${_}`)}${_}}${_}from${_}${pathWithAssertion}`
 				);
@@ -100,7 +104,9 @@ function getImportBlock(
 				for (const specifier of namespaceReexports) {
 					importBlock.push(
 						`export${_}{${_}${
-							name === specifier.reexported ? name : `${name} as ${specifier.reexported}`
+							name === specifier.reexported
+								? name
+								: `${name} as ${safeExportName(specifier.reexported)}`
 						} };`
 					);
 				}
@@ -110,8 +116,8 @@ function getImportBlock(
 					`export${_}{${_}${namedReexports
 						.map(specifier =>
 							specifier.imported === specifier.reexported
-								? specifier.imported
-								: `${specifier.imported} as ${specifier.reexported}`
+								? safeExportName(specifier.imported)
+								: `${safeExportName(specifier.imported)} as ${safeExportName(specifier.reexported)}`
 						)
 						.join(`,${_}`)}${_}}${_}from${_}${pathWithAssertion}`
 				);
@@ -131,7 +137,7 @@ function getExportBlock(exports: ChunkExports, { _, cnst }: GenerateCodeSnippets
 		exportDeclaration.push(
 			specifier.exported === specifier.local
 				? specifier.local
-				: `${specifier.local} as ${specifier.exported}`
+				: `${specifier.local} as ${safeExportName(specifier.exported)}`
 		);
 	}
 	if (exportDeclaration.length > 0) {

diff --git a/src/utils/generateCodeSnippets.ts b/src/utils/generateCodeSnippets.ts
@@ -1,5 +1,6 @@
 import type { NormalizedOutputOptions } from '../rollup/types';
 import RESERVED_NAMES from './RESERVED_NAMES';
+import { isValidIdentifier } from './isValidIdentifier';
 
 export interface GenerateCodeSnippets {
 	_: string;
@@ -83,8 +84,8 @@ export function getGenerateCodeSnippets({
 	];
 
 	const isValidPropertyName = reservedNamesAsProps
-		? (name: string): boolean => validPropertyName.test(name)
-		: (name: string): boolean => !RESERVED_NAMES.has(name) && validPropertyName.test(name);
+		? isValidIdentifier
+		: (name: string): boolean => !RESERVED_NAMES.has(name) && isValidIdentifier(name);
 
 	return {
 		_,
@@ -130,5 +131,3 @@ export function getGenerateCodeSnippets({
 
 const wrapIfNeeded = (code: string, needsParens: boolean | undefined): string =>
 	needsParens ? `(${code})` : code;
-
-const validPropertyName = /^(?!\d)[\w$]+$/;
diff --git a/src/utils/isValidIdentifier.ts b/src/utils/isValidIdentifier.ts
@@ -0,0 +1,5 @@
+const validIdentifier = /^(?!\d)[\w$]+$/;
+
+export function isValidIdentifier(name: string): boolean {
+	return validIdentifier.test(name);
+}
diff --git a/src/utils/safeName.ts b/src/utils/safeName.ts
@@ -1,15 +1,17 @@
 import RESERVED_NAMES from './RESERVED_NAMES';
 import { toBase64 } from './base64';
+import { isValidIdentifier } from './isValidIdentifier';
 
 export function getSafeName(
 	baseName: string,
 	usedNames: Set<string>,
 	forbiddenNames: Set<string> | null
 ): string {
-	let safeName = baseName;
+	const safeBase = isValidIdentifier(baseName) ? baseName : '_safe';
+	let safeName = safeBase;
 	let count = 1;
 	while (usedNames.has(safeName) || RESERVED_NAMES.has(safeName) || forbiddenNames?.has(safeName)) {
-		safeName = `${baseName}$${toBase64(count++)}`;
+		safeName = `${safeBase}$${toBase64(count++)}`;
 	}
 	usedNames.add(safeName);
 	return safeName;

diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/_config.js b/test/form/samples/illegal-identifiers-in-imports-exports/_config.js
@@ -0,0 +1,10 @@
+module.exports = {
+	description: 'correctly handles illegal identifiers in exports/imports',
+	options: {
+		input: ['main'],
+		output: {
+			name: 'illegalIdentifiers'
+		},
+		external: ['external']
+	}
+};
diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/_expected/amd.js b/test/form/samples/illegal-identifiers-in-imports-exports/_expected/amd.js
@@ -0,0 +1,41 @@
+define(['exports', 'external'], (function (exports, external) { 'use strict';
+
+	function _interopNamespaceDefault(e) {
+		var n = Object.create(null);
+		if (e) {
+			Object.keys(e).forEach(function (k) {
+				if (k !== 'default') {
+					var d = Object.getOwnPropertyDescriptor(e, k);
+					Object.defineProperty(n, k, d.get ? d : {
+						enumerable: true,
+						get: function () { return e[k]; }
+					});
+				}
+			});
+		}
+		n.default = e;
+		return Object.freeze(n);
+	}
+
+	var external__namespace = /*#__PURE__*/_interopNamespaceDefault(external);
+
+	console.log(external[":"], external["🤷‍♂️"]); // retain those local bindings
+
+	const legal = 10;
+
+	Object.defineProperty(exports, '-', {
+		enumerable: true,
+		get: function () { return external.bar; }
+	});
+	Object.defineProperty(exports, '/', {
+		enumerable: true,
+		get: function () { return external["/"]; }
+	});
+	exports["🍅"] = external__namespace;
+	Object.defineProperty(exports, '😭', {
+		enumerable: true,
+		get: function () { return external["😂"]; }
+	});
+	exports["🔥illegal"] = legal;
+
+}));
diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/_expected/cjs.js b/test/form/samples/illegal-identifiers-in-imports-exports/_expected/cjs.js
@@ -0,0 +1,41 @@
+'use strict';
+
+var external = require('external');
+
+function _interopNamespaceDefault(e) {
+	var n = Object.create(null);
+	if (e) {
+		Object.keys(e).forEach(function (k) {
+			if (k !== 'default') {
+				var d = Object.getOwnPropertyDescriptor(e, k);
+				Object.defineProperty(n, k, d.get ? d : {
+					enumerable: true,
+					get: function () { return e[k]; }
+				});
+			}
+		});
+	}
+	n.default = e;
+	return Object.freeze(n);
+}
+
+var external__namespace = /*#__PURE__*/_interopNamespaceDefault(external);
+
+console.log(external[":"], external["🤷‍♂️"]); // retain those local bindings
+
+const legal = 10;
+
+Object.defineProperty(exports, '-', {
+	enumerable: true,
+	get: function () { return external.bar; }
+});
+Object.defineProperty(exports, '/', {
+	enumerable: true,
+	get: function () { return external["/"]; }
+});
+exports["🍅"] = external__namespace;
+Object.defineProperty(exports, '😭', {
+	enumerable: true,
+	get: function () { return external["😂"]; }
+});
+exports["🔥illegal"] = legal;
diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/_expected/es.js b/test/form/samples/illegal-identifiers-in-imports-exports/_expected/es.js
@@ -0,0 +1,10 @@
+import { ":" as _safe, "🤷‍♂️" as _safe$1 } from 'external';
+import * as external from 'external';
+export { external as "🍅" };
+export { bar as "-", "/", "😂" as "😭" } from 'external';
+
+console.log(_safe, _safe$1); // retain those local bindings
+
+const legal = 10;
+
+export { legal as "🔥illegal" };
diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/_expected/iife.js b/test/form/samples/illegal-identifiers-in-imports-exports/_expected/iife.js
@@ -0,0 +1,44 @@
+var illegalIdentifiers = (function (exports, external) {
+	'use strict';
+
+	function _interopNamespaceDefault(e) {
+		var n = Object.create(null);
+		if (e) {
+			Object.keys(e).forEach(function (k) {
+				if (k !== 'default') {
+					var d = Object.getOwnPropertyDescriptor(e, k);
+					Object.defineProperty(n, k, d.get ? d : {
+						enumerable: true,
+						get: function () { return e[k]; }
+					});
+				}
+			});
+		}
+		n.default = e;
+		return Object.freeze(n);
+	}
+
+	var external__namespace = /*#__PURE__*/_interopNamespaceDefault(external);
+
+	console.log(external[":"], external["🤷‍♂️"]); // retain those local bindings
+
+	const legal = 10;
+
+	Object.defineProperty(exports, '-', {
+		enumerable: true,
+		get: function () { return external.bar; }
+	});
+	Object.defineProperty(exports, '/', {
+		enumerable: true,
+		get: function () { return external["/"]; }
+	});
+	exports["🍅"] = external__namespace;
+	Object.defineProperty(exports, '😭', {
+		enumerable: true,
+		get: function () { return external["😂"]; }
+	});
+	exports["🔥illegal"] = legal;
+
+	return exports;
+
+})({}, external);
diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/_expected/system.js b/test/form/samples/illegal-identifiers-in-imports-exports/_expected/system.js
@@ -0,0 +1,18 @@
+System.register('illegalIdentifiers', ['external'], (function (exports) {
+	'use strict';
+	var _safe, _safe$1;
+	return {
+		setters: [function (module) {
+			_safe = module[":"];
+			_safe$1 = module["🤷‍♂️"];
+			exports({ '-': module.bar, '/': module["/"], '🍅': module, '😭': module["😂"] });
+		}],
+		execute: (function () {
+
+			console.log(_safe, _safe$1); // retain those local bindings
+
+			const legal = exports('🔥illegal', 10);
+
+		})
+	};
+}));
diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/_expected/umd.js b/test/form/samples/illegal-identifiers-in-imports-exports/_expected/umd.js
@@ -0,0 +1,45 @@
+(function (global, factory) {
+	typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('external')) :
+	typeof define === 'function' && define.amd ? define(['exports', 'external'], factory) :
+	(global = typeof globalThis !== 'undefined' ? globalThis : global || self, factory(global.illegalIdentifiers = {}, global.external));
+})(this, (function (exports, external) { 'use strict';
+
+	function _interopNamespaceDefault(e) {
+		var n = Object.create(null);
+		if (e) {
+			Object.keys(e).forEach(function (k) {
+				if (k !== 'default') {
+					var d = Object.getOwnPropertyDescriptor(e, k);
+					Object.defineProperty(n, k, d.get ? d : {
+						enumerable: true,
+						get: function () { return e[k]; }
+					});
+				}
+			});
+		}
+		n.default = e;
+		return Object.freeze(n);
+	}
+
+	var external__namespace = /*#__PURE__*/_interopNamespaceDefault(external);
+
+	console.log(external[":"], external["🤷‍♂️"]); // retain those local bindings
+
+	const legal = 10;
+
+	Object.defineProperty(exports, '-', {
+		enumerable: true,
+		get: function () { return external.bar; }
+	});
+	Object.defineProperty(exports, '/', {
+		enumerable: true,
+		get: function () { return external["/"]; }
+	});
+	exports["🍅"] = external__namespace;
+	Object.defineProperty(exports, '😭', {
+		enumerable: true,
+		get: function () { return external["😂"]; }
+	});
+	exports["🔥illegal"] = legal;
+
+}));
diff --git a/test/form/samples/illegal-identifiers-in-imports-exports/main.js b/test/form/samples/illegal-identifiers-in-imports-exports/main.js
@@ -0,0 +1,11 @@
+import { ':' as baz, '🤷‍♂️' as bazinga } from 'external';
+console.log(baz, bazinga); // retain those local bindings
+
+const legal = 10;
+
+export { legal as '🔥illegal' };
+
+export { bar as '-', '/', '😂' as '😭' } from 'external';
+
+import * as lib from 'external';
+export { lib as '🍅' }