Break infinite loops in this.resolve (#4000)

* Get rid of plugin index * Prevent infinite loops when using this.resolve in resolveId * Improve coverage * Add documentation * Make test a little more explicit
rollup · Mar 19, 2021 · d3e6e6d · d3e6e6d
1 parent e2ae914
commit d3e6e6d
Show file tree

Hide file tree

Showing 14 changed files with 303 additions and 163 deletions.
diff --git a/browser/resolveId.ts b/browser/resolveId.ts
@@ -1,21 +1,23 @@
-import { CustomPluginOptions } from '../src/rollup/types';
+import { CustomPluginOptions, Plugin, ResolvedId } from '../src/rollup/types';
 import { PluginDriver } from '../src/utils/PluginDriver';
+import { resolveIdViaPlugins } from '../src/utils/resolveIdViaPlugins';
 import { throwNoFileSystem } from './error';
 
 export async function resolveId(
 	source: string,
 	importer: string | undefined,
 	_preserveSymlinks: boolean,
 	pluginDriver: PluginDriver,
-	skip: number | null,
+	moduleLoaderResolveId: (
+		source: string,
+		importer: string | undefined,
+		customOptions: CustomPluginOptions | undefined,
+		skip: { importer: string | undefined; plugin: Plugin; source: string }[] | null
+	) => Promise<ResolvedId | null>,
+	skip: { importer: string | undefined; plugin: Plugin; source: string }[] | null,
 	customOptions: CustomPluginOptions | undefined
 ) {
-	const pluginResult = await pluginDriver.hookFirst(
-		'resolveId',
-		[source, importer, { custom: customOptions }],
-		null,
-		skip
-	);
+	const pluginResult = await resolveIdViaPlugins(source, importer,pluginDriver, moduleLoaderResolveId, skip, customOptions);
 	if (pluginResult == null) {
 		throwNoFileSystem('path.resolve');
 	}

diff --git a/docs/05-plugin-development.md b/docs/05-plugin-development.md
@@ -697,7 +697,7 @@ Use Rollup's internal acorn instance to parse code to an AST.
 #### `this.resolve(source: string, importer?: string, options?: {skipSelf?: boolean, custom?: {[plugin: string]: any}}) => Promise<{id: string, external: boolean, moduleSideEffects: boolean | 'no-treeshake', syntheticNamedExports: boolean | string, meta: {[plugin: string]: any}} | null>`
 Resolve imports to module ids (i.e. file names) using the same plugins that Rollup uses, and determine if an import should be external. If `null` is returned, the import could not be resolved by Rollup or any plugin but was not explicitly marked as external by the user.
 
-If you pass `skipSelf: true`, then the `resolveId` hook of the plugin from which `this.resolve` is called will be skipped when resolving.
+If you pass `skipSelf: true`, then the `resolveId` hook of the plugin from which `this.resolve` is called will be skipped when resolving. When other plugins themselves also call `this.resolve` in their `resolveId` hooks with the *exact same `source` and `importer`* while handling the original `this.resolve` call, then the `resolveId` hook of the original plugin will be skipped for those calls as well. The rationale here is that the plugin already stated that it "does not know" how to resolve this particular combination of `source` and `importer` at this point in time. If you do not want this behaviour, do not use `skipSelf` but implement your own infinite loop prevention mechanism if necessary.
 
 You can also pass an object of plugin-specific options via the `custom` option, see [custom resolver options](guide/en/#custom-resolver-options) for details.
 

diff --git a/src/ModuleLoader.ts b/src/ModuleLoader.ts
@@ -8,6 +8,7 @@ import {
 	HasModuleSideEffects,
 	NormalizedInputOptions,
 	PartialResolvedId,
+	Plugin,
 	ResolvedId,
 	ResolveIdResult,
 	SourceDescription
@@ -135,12 +136,12 @@ export class ModuleLoader {
 		return module;
 	}
 
-	async resolveId(
+	resolveId = async (
 		source: string,
 		importer: string | undefined,
 		customOptions: CustomPluginOptions | undefined,
-		skip: number | null = null
-	): Promise<ResolvedId | null> {
+		skip: { importer: string | undefined; plugin: Plugin; source: string }[] | null = null
+	): Promise<ResolvedId | null> => {
 		return this.addDefaultsToResolvedId(
 			this.getNormalizedResolvedIdWithoutDefaults(
 				this.options.external(source, importer, false)
@@ -150,6 +151,7 @@ export class ModuleLoader {
 							importer,
 							this.options.preserveSymlinks,
 							this.pluginDriver,
+							this.resolveId,
 							skip,
 							customOptions
 					  ),
@@ -444,6 +446,7 @@ export class ModuleLoader {
 			importer,
 			this.options.preserveSymlinks,
 			this.pluginDriver,
+			this.resolveId,
 			null,
 			EMPTY_OBJECT
 		);

diff --git a/src/utils/PluginContext.ts b/src/utils/PluginContext.ts
@@ -44,121 +44,126 @@ function getDeprecatedContextHandler<H extends Function>(
 	}) as unknown) as H;
 }
 
-export function getPluginContexts(
+export function getPluginContext(
+	plugin: Plugin,
 	pluginCache: Record<string, SerializablePluginCache> | void,
 	graph: Graph,
 	options: NormalizedInputOptions,
-	fileEmitter: FileEmitter
-): (plugin: Plugin, pluginIndex: number) => PluginContext {
-	const existingPluginNames = new Set<string>();
-	return (plugin, pidx) => {
-		let cacheable = true;
-		if (typeof plugin.cacheKey !== 'string') {
-			if (
-				plugin.name.startsWith(ANONYMOUS_PLUGIN_PREFIX) ||
-				plugin.name.startsWith(ANONYMOUS_OUTPUT_PLUGIN_PREFIX) ||
-				existingPluginNames.has(plugin.name)
-			) {
-				cacheable = false;
-			} else {
-				existingPluginNames.add(plugin.name);
-			}
-		}
-
-		let cacheInstance: PluginCache;
-		if (!pluginCache) {
-			cacheInstance = NO_CACHE;
-		} else if (cacheable) {
-			const cacheKey = plugin.cacheKey || plugin.name;
-			cacheInstance = createPluginCache(
-				pluginCache[cacheKey] || (pluginCache[cacheKey] = Object.create(null))
-			);
+	fileEmitter: FileEmitter,
+	existingPluginNames: Set<string>
+): PluginContext {
+	let cacheable = true;
+	if (typeof plugin.cacheKey !== 'string') {
+		if (
+			plugin.name.startsWith(ANONYMOUS_PLUGIN_PREFIX) ||
+			plugin.name.startsWith(ANONYMOUS_OUTPUT_PLUGIN_PREFIX) ||
+			existingPluginNames.has(plugin.name)
+		) {
+			cacheable = false;
 		} else {
-			cacheInstance = getCacheForUncacheablePlugin(plugin.name);
+			existingPluginNames.add(plugin.name);
 		}
+	}
 
-		const context: PluginContext = {
-			addWatchFile(id) {
-				if (graph.phase >= BuildPhase.GENERATE) {
-					return this.error(errInvalidRollupPhaseForAddWatchFile());
-				}
-				graph.watchFiles[id] = true;
-			},
-			cache: cacheInstance,
-			emitAsset: getDeprecatedContextHandler(
-				(name: string, source?: string | Uint8Array) =>
-					fileEmitter.emitFile({ type: 'asset', name, source }),
-				'emitAsset',
-				'emitFile',
-				plugin.name,
-				true,
-				options
-			),
-			emitChunk: getDeprecatedContextHandler(
-				(id: string, options?: { name?: string }) =>
-					fileEmitter.emitFile({ type: 'chunk', id, name: options && options.name }),
-				'emitChunk',
-				'emitFile',
-				plugin.name,
-				true,
-				options
-			),
-			emitFile: fileEmitter.emitFile,
-			error(err): never {
-				return throwPluginError(err, plugin.name);
-			},
-			getAssetFileName: getDeprecatedContextHandler(
-				fileEmitter.getFileName,
-				'getAssetFileName',
-				'getFileName',
-				plugin.name,
-				true,
-				options
-			),
-			getChunkFileName: getDeprecatedContextHandler(
-				fileEmitter.getFileName,
-				'getChunkFileName',
-				'getFileName',
-				plugin.name,
-				true,
-				options
-			),
-			getFileName: fileEmitter.getFileName,
-			getModuleIds: () => graph.modulesById.keys(),
-			getModuleInfo: graph.getModuleInfo,
-			getWatchFiles: () => Object.keys(graph.watchFiles),
-			isExternal: getDeprecatedContextHandler(
-				(id: string, parentId: string | undefined, isResolved = false) =>
-					options.external(id, parentId, isResolved),
-				'isExternal',
-				'resolve',
-				plugin.name,
-				true,
-				options
-			),
-			meta: {
-				rollupVersion,
-				watchMode: graph.watchMode
-			},
-			get moduleIds() {
-				function* wrappedModuleIds() {
-					warnDeprecation(
-						{
-							message: `Accessing "this.moduleIds" on the plugin context by plugin ${plugin.name} is deprecated. The "this.getModuleIds" plugin context function should be used instead.`,
-							plugin: plugin.name
-						},
-						false,
-						options
-					);
-					yield* moduleIds;
-				}
+	let cacheInstance: PluginCache;
+	if (!pluginCache) {
+		cacheInstance = NO_CACHE;
+	} else if (cacheable) {
+		const cacheKey = plugin.cacheKey || plugin.name;
+		cacheInstance = createPluginCache(
+			pluginCache[cacheKey] || (pluginCache[cacheKey] = Object.create(null))
+		);
+	} else {
+		cacheInstance = getCacheForUncacheablePlugin(plugin.name);
+	}
+
+	const context: PluginContext = {
+		addWatchFile(id) {
+			if (graph.phase >= BuildPhase.GENERATE) {
+				return this.error(errInvalidRollupPhaseForAddWatchFile());
+			}
+			graph.watchFiles[id] = true;
+		},
+		cache: cacheInstance,
+		emitAsset: getDeprecatedContextHandler(
+			(name: string, source?: string | Uint8Array) =>
+				fileEmitter.emitFile({ type: 'asset', name, source }),
+			'emitAsset',
+			'emitFile',
+			plugin.name,
+			true,
+			options
+		),
+		emitChunk: getDeprecatedContextHandler(
+			(id: string, options?: { name?: string }) =>
+				fileEmitter.emitFile({ type: 'chunk', id, name: options && options.name }),
+			'emitChunk',
+			'emitFile',
+			plugin.name,
+			true,
+			options
+		),
+		emitFile: fileEmitter.emitFile,
+		error(err): never {
+			return throwPluginError(err, plugin.name);
+		},
+		getAssetFileName: getDeprecatedContextHandler(
+			fileEmitter.getFileName,
+			'getAssetFileName',
+			'getFileName',
+			plugin.name,
+			true,
+			options
+		),
+		getChunkFileName: getDeprecatedContextHandler(
+			fileEmitter.getFileName,
+			'getChunkFileName',
+			'getFileName',
+			plugin.name,
+			true,
+			options
+		),
+		getFileName: fileEmitter.getFileName,
+		getModuleIds: () => graph.modulesById.keys(),
+		getModuleInfo: graph.getModuleInfo,
+		getWatchFiles: () => Object.keys(graph.watchFiles),
+		isExternal: getDeprecatedContextHandler(
+			(id: string, parentId: string | undefined, isResolved = false) =>
+				options.external(id, parentId, isResolved),
+			'isExternal',
+			'resolve',
+			plugin.name,
+			true,
+			options
+		),
+		meta: {
+			rollupVersion,
+			watchMode: graph.watchMode
+		},
+		get moduleIds() {
+			function* wrappedModuleIds() {
+				warnDeprecation(
+					{
+						message: `Accessing "this.moduleIds" on the plugin context by plugin ${plugin.name} is deprecated. The "this.getModuleIds" plugin context function should be used instead.`,
+						plugin: plugin.name
+					},
+					false,
+					options
+				);
+				yield* moduleIds;
+			}
 
 				const moduleIds = graph.modulesById.keys();
 				return wrappedModuleIds();
 			},
 			parse: graph.contextParse.bind(graph),
 			resolve(source, importer, { custom, skipSelf } = BLANK) {
-				return graph.moduleLoader.resolveId(source, importer, custom, skipSelf ? pidx : null);
+				return graph.moduleLoader.resolveId(
+					source,
+					importer,
+					custom,
+					skipSelf ? [{ importer, plugin, source }] : null
+				);
 			},
 			resolveId: getDeprecatedContextHandler(
 				(source: string, importer: string | undefined) =>
@@ -181,5 +186,4 @@ export function getPluginContexts(
 			}
 		};
 		return context;
-	};
 }