Keep attributes for external dynamic imports (#6071)

TrickyPi · web-flow · commit 94c96fa90589 · 2025-08-23T05:28:59.000Z
* Add test

* Binding attributes for unresolved dynamic imports

* Improve the implementation

* Move this.options.includePath into includeNode
diff --git a/src/Chunk.ts b/src/Chunk.ts
@@ -1018,15 +1018,19 @@ export default class Chunk {
 		fileName: string,
 		node: ImportExpression
 	): [importPath: string, attributes: string | null | true] {
+		const { externalImportAttributes } = this.outputOptions;
+		const keepExternalImportAttributes =
+			['es', 'cjs'].includes(this.outputOptions.format) && externalImportAttributes;
 		if (resolution instanceof ExternalModule) {
 			const chunk = this.externalChunkByModule.get(resolution)!;
-			return [`'${chunk.getImportPath(fileName)}'`, chunk.getImportAttributes(this.snippets)];
+			const dynamicAttributes = chunk.getImportAttributes(this.snippets);
+			return [
+				`'${chunk.getImportPath(fileName)}'`,
+				dynamicAttributes || (keepExternalImportAttributes ? true : null)
+			];
 		}
 		let attributes: string | true | null = null;
-		if (
-			['es', 'cjs'].includes(this.outputOptions.format) &&
-			this.outputOptions.externalImportAttributes
-		) {
+		if (keepExternalImportAttributes) {
 			const attributesFromImportAttributes = getAttributesFromImportExpression(node);
 			attributes =
 				attributesFromImportAttributes === EMPTY_OBJECT
diff --git a/src/ModuleLoader.ts b/src/ModuleLoader.ts
@@ -567,7 +567,10 @@ export class ModuleLoader {
 				module.id,
 				getAttributesFromImportExpression(dynamicImport.node)
 			);
-			if (resolvedId && typeof resolvedId === 'object') {
+			if (!resolvedId || typeof resolvedId === 'string') {
+				dynamicImport.node.shouldIncludeDynamicAttributes = true;
+			} else {
+				dynamicImport.node.shouldIncludeDynamicAttributes = !!resolvedId.external;
 				dynamicImport.id = resolvedId.id;
 			}
 			return [dynamicImport, resolvedId] as const;
diff --git a/src/ast/nodes/ImportExpression.ts b/src/ast/nodes/ImportExpression.ts
@@ -21,7 +21,7 @@ import { findFirstOccurrenceOutsideComment, type RenderOptions } from '../../uti
 import type { InclusionContext } from '../ExecutionContext';
 import type ChildScope from '../scopes/ChildScope';
 import type { ObjectPath } from '../utils/PathTracker';
-import { UnknownKey } from '../utils/PathTracker';
+import { UNKNOWN_PATH, UnknownKey } from '../utils/PathTracker';
 import type NamespaceVariable from '../variables/NamespaceVariable';
 import ArrowFunctionExpression from './ArrowFunctionExpression';
 import AwaitExpression from './AwaitExpression';
@@ -68,6 +68,14 @@ export default class ImportExpression extends NodeBase {
 	private resolution: Module | ExternalModule | string | null = null;
 	private resolutionString: string | null = null;
 
+	get shouldIncludeDynamicAttributes() {
+		return isFlagSet(this.flags, Flag.shouldIncludeDynamicAttributes);
+	}
+
+	set shouldIncludeDynamicAttributes(value: boolean) {
+		this.flags = setFlag(this.flags, Flag.shouldIncludeDynamicAttributes, value);
+	}
+
 	get withinTopLevelAwait() {
 		return isFlagSet(this.flags, Flag.withinTopLevelAwait);
 	}
@@ -76,9 +84,9 @@ export default class ImportExpression extends NodeBase {
 		this.flags = setFlag(this.flags, Flag.withinTopLevelAwait, value);
 	}
 
-	// Do not bind attributes
 	bind(): void {
 		this.source.bind();
+		this.options?.bind();
 	}
 
 	/**
@@ -185,18 +193,21 @@ export default class ImportExpression extends NodeBase {
 	}
 
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void {
-		if (!this.included) this.includeNode();
+		if (!this.included) this.includeNode(context);
 		this.source.include(context, includeChildrenRecursively);
+		if (this.shouldIncludeDynamicAttributes)
+			this.options?.include(context, includeChildrenRecursively);
 	}
 
-	includeNode() {
+	includeNode(context: InclusionContext) {
 		this.included = true;
+		if (this.shouldIncludeDynamicAttributes) this.options?.includePath(UNKNOWN_PATH, context);
 		this.scope.context.includeDynamicImport(this);
 		this.scope.addAccessedDynamicImport(this);
 	}
 
-	includePath(path: ObjectPath): void {
-		if (!this.included) this.includeNode();
+	includePath(path: ObjectPath, context: InclusionContext): void {
+		if (!this.included) this.includeNode(context);
 		// Technically, this is not correct as dynamic imports return a Promise.
 		if (this.hasUnknownAccessedKey) return;
 		if (path[0] === UnknownKey) {
diff --git a/src/ast/nodes/shared/BitFlags.ts b/src/ast/nodes/shared/BitFlags.ts
@@ -27,7 +27,8 @@ export const enum Flag {
 	hasDeoptimizedCache = 1 << 25,
 	hasEffects = 1 << 26,
 	withinTopLevelAwait = 1 << 27,
-	checkedForWarnings = 1 << 28
+	checkedForWarnings = 1 << 28,
+	shouldIncludeDynamicAttributes = 1 << 29
 }
 
 export function isFlagSet(flags: number, flag: Flag): boolean {
diff --git a/src/ast/variables/LocalVariable.ts b/src/ast/variables/LocalVariable.ts
@@ -246,7 +246,7 @@ export default class LocalVariable extends Variable {
 					declaration.parent.parent.callee.property.name === 'then' &&
 					isImportExpressionNode(declaration.parent.parent.callee.object)
 				) {
-					declaration.parent.parent.callee.object.includePath(path);
+					declaration.parent.parent.callee.object.includePath(path, context);
 				}
 			}
 			// We need to make sure we include the correct path of the init
diff --git a/src/utils/logs.ts b/src/utils/logs.ts
@@ -608,7 +608,7 @@ export function logImportAttributeIsInvalid(importer: string): RollupLog {
 		code: INVALID_IMPORT_ATTRIBUTE,
 		message: `Rollup could not statically analyze an import attribute of a dynamic import in "${relativeId(
 			importer
-		)}". Import attributes need to have string keys and values. The attribute will be removed.`
+		)}". Import attributes need to have string keys and values.`
 	};
 }
 
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_config.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_config.js
@@ -0,0 +1,14 @@
+module.exports = defineTest({
+	description: 'Remove the attribute declarations for internal dynamic imports',
+	expectedWarnings: ['INVALID_IMPORT_ATTRIBUTE'],
+	options: {
+		plugins: [
+			{
+				resolveDynamicImport() {
+					return { id: './foo.js' };
+				}
+			}
+		]
+	},
+	verifyAst: false
+});
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/amd/generated-foo.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/amd/generated-foo.js
@@ -0,0 +1,8 @@
+define(['exports'], (function (exports) { 'use strict';
+
+	const foo = 'foo';
+	console.log(foo);
+
+	exports.foo = foo;
+
+}));
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/amd/main.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/amd/main.js
@@ -0,0 +1,5 @@
+define(['require'], (function (require) { 'use strict';
+
+	new Promise(function (resolve, reject) { require(['./generated-foo'], resolve, reject); });
+
+}));
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/cjs/generated-foo.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/cjs/generated-foo.js
@@ -0,0 +1,6 @@
+'use strict';
+
+const foo = 'foo';
+console.log(foo);
+
+exports.foo = foo;
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/cjs/main.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/cjs/main.js
@@ -0,0 +1,3 @@
+'use strict';
+
+Promise.resolve().then(function () { return require('./generated-foo.js'); });
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/es/generated-foo.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/es/generated-foo.js
@@ -0,0 +1,4 @@
+const foo = 'foo';
+console.log(foo);
+
+export { foo };
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/es/main.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/es/main.js
@@ -0,0 +1 @@
+import('./generated-foo.js');
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/system/generated-foo.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/system/generated-foo.js
@@ -0,0 +1,11 @@
+System.register([], (function (exports) {
+	'use strict';
+	return {
+		execute: (function () {
+
+			const foo = exports("foo", 'foo');
+			console.log(foo);
+
+		})
+	};
+}));
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/system/main.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/_expected/system/main.js
@@ -0,0 +1,10 @@
+System.register([], (function (exports, module) {
+	'use strict';
+	return {
+		execute: (function () {
+
+			module.import('./generated-foo.js');
+
+		})
+	};
+}));
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/foo.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/foo.js
@@ -0,0 +1,2 @@
+export const foo = 'foo';
+console.log(foo);
diff --git a/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/main.js b/test/chunking-form/samples/remove-attribute-declarations-for-internal-dynamic-imports/main.js
@@ -0,0 +1,3 @@
+const extra = { with: { type: 'json' } };
+import(specifier, extra);
+
diff --git a/test/form/samples/import-attributes/attribute-shapes/_expected.js b/test/form/samples/import-attributes/attribute-shapes/_expected.js
@@ -1,4 +1,4 @@
 import('external-a', { assert: { type: 'json' } });
-import('external-b');
-import('external-c');
-import('external-d');
+import('external-b', { with: {} });
+import('external-c', { ...{} });
+import('external-d', {});
diff --git a/test/form/samples/import-attributes/keep-attribute-declarations-for-external-dynamic-imports/_config.js b/test/form/samples/import-attributes/keep-attribute-declarations-for-external-dynamic-imports/_config.js
@@ -0,0 +1,19 @@
+module.exports = defineTest({
+	description: 'Keep the attribute declarations for external dynamic imports',
+	expectedWarnings: ['INVALID_IMPORT_ATTRIBUTE'],
+	options: {
+		plugins: [
+			{
+				resolveDynamicImport(specifier) {
+					if (specifier === 'specifier3' || specifier === 'specifier4') {
+						return { id: specifier, external: true };
+					}
+					if (specifier.name === 'specifier5' || specifier.name === 'specifier6') {
+						return `'${specifier.name}-replacement'`;
+					}
+				}
+			}
+		]
+	},
+	verifyAst: false
+});
diff --git a/test/form/samples/import-attributes/keep-attribute-declarations-for-external-dynamic-imports/_expected.js b/test/form/samples/import-attributes/keep-attribute-declarations-for-external-dynamic-imports/_expected.js
@@ -0,0 +1,17 @@
+const extra1 = { with: { type: 'json' } };
+import(specifier1, extra1);
+
+const extra2 = { with: { type: 'json' } };
+import(specifier2, { with: extra2.with });
+
+const extra3 = { with: { type: 'json' } };
+import('specifier3', extra3);
+
+const extra4 = { with: { type: 'json' } };
+import('specifier4', { with: extra4.with });
+
+const extra5 = { with: { type: 'json' } };
+import('specifier5-replacement', extra5);
+
+const extra6 = { with: { type: 'json' } };
+import('specifier6-replacement', { with: extra6.with });
diff --git a/test/form/samples/import-attributes/keep-attribute-declarations-for-external-dynamic-imports/main.js b/test/form/samples/import-attributes/keep-attribute-declarations-for-external-dynamic-imports/main.js
@@ -0,0 +1,17 @@
+const extra1 = { with: { type: 'json' } };
+import(specifier1, extra1);
+
+const extra2 = { with: { type: 'json' } };
+import(specifier2, { with: extra2.with });
+
+const extra3 = { with: { type: 'json' } };
+import('specifier3', extra3);
+
+const extra4 = { with: { type: 'json' } };
+import('specifier4', { with: extra4.with });
+
+const extra5 = { with: { type: 'json' } };
+import(specifier5, extra5);
+
+const extra6 = { with: { type: 'json' } };
+import(specifier6, { with: extra6.with });
diff --git a/test/function/samples/import-assertions/warn-unresolvable-assertions/_config.js b/test/function/samples/import-assertions/warn-unresolvable-assertions/_config.js
@@ -4,14 +4,19 @@ const ID_MAIN = path.join(__dirname, 'main.js');
 module.exports = defineTest({
 	description: 'warns for dynamic import attributes that cannot be resolved',
 	verifyAst: false,
+	context: {
+		bar: 'bar',
+		foo: 'foo',
+		invalid: 'invalid'
+	},
 	options: {
 		external: id => id.startsWith('external')
 	},
 	warnings: [
 		{
 			code: 'INVALID_IMPORT_ATTRIBUTE',
 			message:
-				'main.js (1:0): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values. The attribute will be removed.',
+				'main.js (1:0): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values.',
 			id: ID_MAIN,
 			pos: 0,
 			loc: {
@@ -28,7 +33,7 @@ module.exports = defineTest({
 		{
 			code: 'INVALID_IMPORT_ATTRIBUTE',
 			message:
-				'main.js (2:0): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values. The attribute will be removed.',
+				'main.js (2:0): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values.',
 			id: ID_MAIN,
 			pos: 32,
 			loc: {
@@ -46,7 +51,7 @@ module.exports = defineTest({
 		{
 			code: 'INVALID_IMPORT_ATTRIBUTE',
 			message:
-				'main.js (4:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values. The attribute will be removed.',
+				'main.js (4:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values.',
 			id: ID_MAIN,
 			pos: 133,
 			loc: {
@@ -65,7 +70,7 @@ module.exports = defineTest({
 		{
 			code: 'INVALID_IMPORT_ATTRIBUTE',
 			message:
-				'main.js (5:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values. The attribute will be removed.',
+				'main.js (5:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values.',
 			id: ID_MAIN,
 			pos: 173,
 			loc: {
@@ -84,7 +89,7 @@ module.exports = defineTest({
 		{
 			code: 'INVALID_IMPORT_ATTRIBUTE',
 			message:
-				'main.js (6:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values. The attribute will be removed.',
+				'main.js (6:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values.',
 			id: ID_MAIN,
 			pos: 218,
 			loc: {
@@ -103,7 +108,7 @@ module.exports = defineTest({
 		{
 			code: 'INVALID_IMPORT_ATTRIBUTE',
 			message:
-				'main.js (7:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values. The attribute will be removed.',
+				'main.js (7:30): Rollup could not statically analyze an import attribute of a dynamic import in "main.js". Import attributes need to have string keys and values.',
 			id: ID_MAIN,
 			pos: 263,
 			loc: {

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,8 @@ export const enum Flag {`
`27`	`27`	`hasDeoptimizedCache = 1 << 25,`
`28`	`28`	`hasEffects = 1 << 26,`
`29`	`29`	`withinTopLevelAwait = 1 << 27,`
`30`		`- checkedForWarnings = 1 << 28`
	`30`	`+ checkedForWarnings = 1 << 28,`
	`31`	`+ shouldIncludeDynamicAttributes = 1 << 29`
`31`	`32`	`}`
`32`	`33`
`33`	`34`	`export function isFlagSet(flags: number, flag: Flag): boolean {`
Original file line number	Diff line number	Diff line change
`@@ -246,7 +246,7 @@ export default class LocalVariable extends Variable {`
`246`	`246`	`declaration.parent.parent.callee.property.name === 'then' &&`
`247`	`247`	`isImportExpressionNode(declaration.parent.parent.callee.object)`
`248`	`248`	`) {`
`249`		`- declaration.parent.parent.callee.object.includePath(path);`
	`249`	`+ declaration.parent.parent.callee.object.includePath(path, context);`
`250`	`250`	`}`
`251`	`251`	`}`
`252`	`252`	`// We need to make sure we include the correct path of the init`
Original file line number	Diff line number	Diff line change
`@@ -608,7 +608,7 @@ export function logImportAttributeIsInvalid(importer: string): RollupLog {`
`608`	`608`	`code: INVALID_IMPORT_ATTRIBUTE,`
`609`	`609`	message: `Rollup could not statically analyze an import attribute of a dynamic import in "${relativeId(
`610`	`610`	`importer`
`611`		- )}". Import attributes need to have string keys and values. The attribute will be removed.`
	`611`	+ )}". Import attributes need to have string keys and values.`
`612`	`612`	`};`
`613`	`613`	`}`
`614`	`614`
-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +define(['require'], (function (require) { 'use strict';
++
 +	new Promise(function (resolve, reject) { require(['./generated-foo'], resolve, reject); });
++
 +}));