ModuleInfo.importedIds does not crash in moduleParsed (#4208)

FoxDaxian · lukastaegert · web-flow · commit 05fe93d235b7 · 2021-08-23T07:04:49.000+02:00
* fixed: ModuleInfo.importedIds

moduleInfo.importedIds return null if resolvedIds[source] if undefined

* Only call moduleParsed once importedIds and dynamicallyImportedIds are available

* Update documentation

Co-authored-by: Lukas Taegert-Atkinson &lt;lukas.taegert-atkinson@tngtech.com&gt;
diff --git a/docs/05-plugin-development.md b/docs/05-plugin-development.md
@@ -122,7 +122,7 @@ This hook is called each time a module has been fully parsed by Rollup. See [`th
 
 In contrast to the [`transform`](guide/en/#transform) hook, this hook is never cached and can be used to get information about both cached and other modules, including the final shape of the `meta` property, the `code` and the `ast`.
 
-Note that information about imported modules is not yet available in this hook, and information about importing modules may be incomplete as additional importers could be discovered later. If you need this information, use the [`buildEnd`](guide/en/#buildend) hook.
+This hook will wait until all imports are resolved so that the information in `moduleInfo.importedIds` and `moduleInfo.dynamicallyImportedIds` is complete and accurate. Note however that information about importing modules may be incomplete as additional importers could be discovered later. If you need this information, use the [`buildEnd`](guide/en/#buildend) hook.
 
 #### `options`
 
diff --git a/src/Module.ts b/src/Module.ts
@@ -116,6 +116,13 @@ export interface AstContext {
 	warn: (warning: RollupWarning, pos: number) => void;
 }
 
+export interface DynamicImport {
+	argument: string | ExpressionNode;
+	id: string | null;
+	node: ImportExpression;
+	resolution: Module | ExternalModule | string | null;
+}
+
 const MISSING_EXPORT_SHIM_DESCRIPTION: ExportDescription = {
 	identifier: null,
 	localName: MISSING_EXPORT_SHIM_VARIABLE
@@ -187,11 +194,7 @@ export default class Module {
 	dependencies = new Set<Module | ExternalModule>();
 	dynamicDependencies = new Set<Module | ExternalModule>();
 	dynamicImporters: string[] = [];
-	dynamicImports: {
-		argument: string | ExpressionNode;
-		node: ImportExpression;
-		resolution: Module | ExternalModule | string | null;
-	}[] = [];
+	dynamicImports: DynamicImport[] = [];
 	excludeFromSourcemap: boolean;
 	execIndex = Infinity;
 	exportAllSources = new Set<string>();
@@ -256,9 +259,9 @@ export default class Module {
 			code: null,
 			get dynamicallyImportedIds() {
 				const dynamicallyImportedIds: string[] = [];
-				for (const { resolution } of module.dynamicImports) {
-					if (resolution instanceof Module || resolution instanceof ExternalModule) {
-						dynamicallyImportedIds.push(resolution.id);
+				for (const { id } of module.dynamicImports) {
+					if (id) {
+						dynamicallyImportedIds.push(id);
 					}
 				}
 				return dynamicallyImportedIds;
@@ -846,7 +849,7 @@ export default class Module {
 		} else if (argument instanceof Literal && typeof argument.value === 'string') {
 			argument = argument.value;
 		}
-		this.dynamicImports.push({ argument, node, resolution: null });
+		this.dynamicImports.push({ argument, id: null, node, resolution: null });
 	}
 
 	private addExport(
diff --git a/src/ModuleLoader.ts b/src/ModuleLoader.ts
@@ -1,7 +1,7 @@
 import * as acorn from 'acorn';
 import ExternalModule from './ExternalModule';
 import Graph from './Graph';
-import Module from './Module';
+import Module, { DynamicImport } from './Module';
 import {
 	CustomPluginOptions,
 	EmittedChunk,
@@ -276,27 +276,27 @@ export class ModuleLoader {
 		return loadNewModulesPromise;
 	}
 
-	private async fetchDynamicDependencies(module: Module): Promise<void> {
+	private async fetchDynamicDependencies(
+		module: Module,
+		resolveDynamicImportPromises: Promise<
+			[dynamicImport: DynamicImport, resolvedId: ResolvedId | string | null]
+		>[]
+	): Promise<void> {
 		const dependencies = await Promise.all(
-			module.dynamicImports.map(async dynamicImport => {
-				const resolvedId = await this.resolveDynamicImport(
-					module,
-					typeof dynamicImport.argument === 'string'
-						? dynamicImport.argument
-						: dynamicImport.argument.esTreeNode,
-					module.id
-				);
-				if (resolvedId === null) return null;
-				if (typeof resolvedId === 'string') {
-					dynamicImport.resolution = resolvedId;
-					return null;
-				}
-				return (dynamicImport.resolution = await this.fetchResolvedDependency(
-					relativeId(resolvedId.id),
-					module.id,
-					resolvedId
-				));
-			})
+			resolveDynamicImportPromises.map(resolveDynamicImportPromise =>
+				resolveDynamicImportPromise.then(async ([dynamicImport, resolvedId]) => {
+					if (resolvedId === null) return null;
+					if (typeof resolvedId === 'string') {
+						dynamicImport.resolution = resolvedId;
+						return null;
+					}
+					return (dynamicImport.resolution = await this.fetchResolvedDependency(
+						relativeId(resolvedId.id),
+						module.id,
+						resolvedId
+					));
+				})
+			)
 		);
 		for (const dependency of dependencies) {
 			if (dependency) {
@@ -336,10 +336,15 @@ export class ModuleLoader {
 		this.modulesById.set(id, module);
 		this.graph.watchFiles[id] = true;
 		await this.addModuleSource(id, importer, module);
-		await this.pluginDriver.hookParallel('moduleParsed', [module.info]);
+		const resolveStaticDependencyPromises = this.getResolveStaticDependencyPromises(module);
+		const resolveDynamicImportPromises = this.getResolveDynamicImportPromises(module);
+		Promise.all([
+			...(resolveStaticDependencyPromises as Promise<never>[]),
+			...(resolveDynamicImportPromises as Promise<never>[])
+		]).then(() => this.pluginDriver.hookParallel('moduleParsed', [module.info]));
 		await Promise.all([
-			this.fetchStaticDependencies(module),
-			this.fetchDynamicDependencies(module)
+			this.fetchStaticDependencies(module, resolveStaticDependencyPromises),
+			this.fetchDynamicDependencies(module, resolveDynamicImportPromises)
 		]);
 		module.linkImports();
 		return module;
@@ -375,19 +380,14 @@ export class ModuleLoader {
 		}
 	}
 
-	private async fetchStaticDependencies(module: Module): Promise<void> {
+	private async fetchStaticDependencies(
+		module: Module,
+		resolveStaticDependencyPromises: Promise<[source: string, resolvedId: ResolvedId]>[]
+	): Promise<void> {
 		for (const dependency of await Promise.all(
-			Array.from(module.sources, async source =>
-				this.fetchResolvedDependency(
-					source,
-					module.id,
-					(module.resolvedIds[source] =
-						module.resolvedIds[source] ||
-						this.handleResolveId(
-							await this.resolveId(source, module.id, EMPTY_OBJECT),
-							source,
-							module.id
-						))
+			resolveStaticDependencyPromises.map(resolveStaticDependencyPromise =>
+				resolveStaticDependencyPromise.then(([source, resolvedId]) =>
+					this.fetchResolvedDependency(source, module.id, resolvedId)
 				)
 			)
 		)) {
@@ -450,6 +450,43 @@ export class ModuleLoader {
 		};
 	}
 
+	private getResolveDynamicImportPromises(
+		module: Module
+	): Promise<[dynamicImport: DynamicImport, resolvedId: ResolvedId | string | null]>[] {
+		return module.dynamicImports.map(async dynamicImport => {
+			const resolvedId = await this.resolveDynamicImport(
+				module,
+				typeof dynamicImport.argument === 'string'
+					? dynamicImport.argument
+					: dynamicImport.argument.esTreeNode,
+				module.id
+			);
+			if (resolvedId && typeof resolvedId === 'object') {
+				dynamicImport.id = resolvedId.id;
+			}
+			return [dynamicImport, resolvedId] as [DynamicImport, ResolvedId | string | null];
+		});
+	}
+
+	private getResolveStaticDependencyPromises(
+		module: Module
+	): Promise<[source: string, resolvedId: ResolvedId]>[] {
+		return Array.from(
+			module.sources,
+			async source =>
+				[
+					source,
+					(module.resolvedIds[source] =
+						module.resolvedIds[source] ||
+						this.handleResolveId(
+							await this.resolveId(source, module.id, EMPTY_OBJECT),
+							source,
+							module.id
+						))
+				] as [string, ResolvedId]
+		);
+	}
+
 	private handleResolveId(
 		resolvedId: ResolvedId | null,
 		source: string,
diff --git a/test/function/samples/module-parsed-imported-ids/_config.js b/test/function/samples/module-parsed-imported-ids/_config.js
@@ -0,0 +1,31 @@
+const assert = require('assert');
+const path = require('path');
+
+module.exports = {
+	description: 'provides full importedIds and dynamicallyImportedIds in the moduleParsed hook',
+	options: {
+		plugins: [
+			{
+				load(id) {
+					const { dynamicallyImportedIds, importedIds } = this.getModuleInfo(id);
+					assert.deepStrictEqual(importedIds, []);
+					assert.deepStrictEqual(dynamicallyImportedIds, []);
+				},
+				transform(code, id) {
+					const { dynamicallyImportedIds, importedIds } = this.getModuleInfo(id);
+					assert.deepStrictEqual(importedIds, []);
+					assert.deepStrictEqual(dynamicallyImportedIds, []);
+				},
+				moduleParsed({ dynamicallyImportedIds, id, importedIds }) {
+					if (id.endsWith('main.js')) {
+						assert.deepStrictEqual(importedIds, [path.join(__dirname, 'static.js')]);
+						assert.deepStrictEqual(dynamicallyImportedIds, [path.join(__dirname, 'dynamic.js')]);
+					} else {
+						assert.deepStrictEqual(importedIds, []);
+						assert.deepStrictEqual(dynamicallyImportedIds, []);
+					}
+				}
+			}
+		]
+	}
+};
diff --git a/test/function/samples/module-parsed-imported-ids/dynamic.js b/test/function/samples/module-parsed-imported-ids/dynamic.js
@@ -0,0 +1 @@
+export const bar = 'dynamic';
diff --git a/test/function/samples/module-parsed-imported-ids/main.js b/test/function/samples/module-parsed-imported-ids/main.js
@@ -0,0 +1,3 @@
+import { foo } from './static.js';
+
+export const result = [foo, import('./dynamic.js')];
diff --git a/test/function/samples/module-parsed-imported-ids/static.js b/test/function/samples/module-parsed-imported-ids/static.js
@@ -0,0 +1 @@
+export const foo = 'static';

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+import { foo } from './static.js';`
	`2`	`+`
	`3`	`+export const result = [foo, import('./dynamic.js')];`