[v4.0] Set the default of skipSelf to true (#5142)

* Set minimum Node version to 18 * Set the default of skipSelf to true * [v4.0] Switch parser to SWC and introduce native/WASM code (#5073) * Add native compilation for local NodeJS * Link instead of copying for faster dev cycles for now * Parse AST * Convert first trivial AST to a buffer * Use SWC for parsing * Extend AST conversion * Make AST more similar * Fix line number issue by creating a new sourcemap (and thus compiler) for each run * Collect timings * Add code_length to struct * Refine parsing * Extend parsing * Extend AST: ImportDefaultSpecifier, LiteralBoolean, LiteralNull, ExportDefaultExpression * Extend AST: ImportNamespaceSpecifier, ExportAll * Extend AST: BinaryExpression, ArrayPattern, ObjectPattern, AssignmentPatternProperty, ArrayLiteral, ImportExpression * Extend AST: ConditionalExpression * Extend AST: FunctionDeclaration, ClassDeclaration, ClassBody, ReturnStatement * Extend AST: ObjectLiteral, KeyValueProperty * Extend AST: ShorthandProperty * Extend AST: GetterProperty, AssignmentExpression, NewExpression, FunctionExpression * Extend AST: ThrowStatement * Extend AST: ExportDefaultDeclaration * Extend AST: AssignmentPattern, AwaitExpression, BreakStatement Start sorting AST nodes * Extend AST: TryStatement, CatchClause, ChainExpression * Extend AST: ClassExpression, ContinueStatement, DebuggerStatement, DoWhileStatement, EmptyStatement * Extend AST: ExportNamedDeclaration, ForInStatement, ForOfStatement, ForStatement * Extend AST: IfStatement * Extend AST: Import attributes * Extend AST: Literal<RegExp>, Literal<BigInt> * Extend AST: LogicalExpression * Extend AST: MetaProperty * Extend AST: Various Property types * Extend AST: Progress on classes * Extend AST: MethodDefinition, PropertyDefinition, PrivateName, ThisExpression * Extend AST: StaticBlock, Super * Extend AST: RestElement, SequenceExpression, SwitchCase, SwitchStatement * Extend AST: TaggedTemplateExpression, TemplateElement, TemplateLiteral, UnaryExpression, UpdateExpression, YieldExpression * Extend AST: Properties in object patterns * Finishing Fixes * More fixes * Run cargo fmt * Handle directives * Minor fixes * Unicode support * Fix optional chain expressions * Adapt tests * Do not run acorn anymore * Update lockfile * Minor fixes * Move to rust folder * Separate Rust Node bindings to allow adding WASM bindings in another workspace * Make Napi build closer to how rs.napi works * Fix path issues * Disable browser build for now * Add native package directories * Refine runWithEcho * Try initial steps with Github flow * Trigger change * Temporarily add yarn lockfile until we figured out if we get it to work with npm * Use nightly toolchain * Use default locations for Napi files to make things easier * Adapt workflow * Skip regular tests for now * Attempt to fix broken workflows * Attempt to fix broken workflows * Attempt to fix broken workflows * Test MacOS/WIndows * Fix bootstrap build * Skip tests * Rename workflow * Add additional tests * Use zig differently * Try to fix musl build * Skip musl again for now * Add publish to workflow * 4.0.0-0 * Remove yarn lock again * Fix coverage job * Fix artefact handling * Revert "4.0.0-0" This reverts commit 734806f. * 4.0.0-0 * Do not include default triples twice * Fix native npm packages * 4.0.0-1 * Add missing additional tests except browser tests * Try to fix publish and tests * 4.0.0-2 * Switch to faster utf-16 conversion * 4.0.0-3 * Fix positioning algorithm when manually searching the code * 4.0.0-4 * feat: Add WASM browser build (#5077) * feat: add wasm browser build * move wasm binding into a separate cargo workspace * use imports replacing * set the targetEnv option of wasm to browser in browser build * add the wasm build to build command * fix lint error and ci error * add more comments to silence the linter * big change * trigger change * run browser tests * trigger change * tweak wasm build on CI * Increase build timeout * Use shared string constants * Extract fixed strings into constants * Remove comment * Get rid of dbg! calls * Add lockfile hash to cargo cache * Use if let over match in some cases * Return the buffer of the syntax error in parse_ast * Initial annotation support * Put annotation types into string table * Remove invalid annotations * Support nested calls and new expressions * Improve tree-shaking for annotations * Adapt test * Properly handle line-breaks, commas etc. * Mark nested pure annotations as pure * Remove sourcemap comments * Handle function side effect annotatinos * Remove old comment-handling code * Increase timeout for browser tests * Only skip the tests that still fail * Run coverage again on CI * Get the buffer of pos and message from the Syntax error * Handle the lint errors from SWC * Reenable tests about parse errors and adjust some relevant code * Emit native.js to native.cjs * Add cjs extension for native importee when bundling for ESM * Change native importee with replace plugin and emit native.cjs in napi build * Silence the linter for importing native * Use node:path to resolve native binding files * Add Node WASM files to Native packages for StackBlitz and similar use cases * Unignore *.d.ts in the wasm dir * 4.0.0-5 * Fix copy-wasm-node.js * 4.0.0-6 * Remove .gitignore in wasm-node directory * 4.0.0-7 * Include .cjs files when publishing rollup to npm * 4.0.0-8 * Get readString function at runtime * eslint: ignore wasm-node and set node extensions of import/resolver * 4.0.0-9 * Change the required node to >=14.18.0 as before * 4.0.0-10 * Prepare to fix ESM build Still requires bug-fix on Rollup side for relative external dependencies outside the ouput directory * Update Rollup * Enable ESM tests * Re-enable another test * Remove CJS eslint configuration * Fix extension * Fix test * Fix entension for native import * Support for publishing a completely separate package @rollup/wasm-node * 4.0.0-11 * Fix publish for wasm node package * Add AST verification to function tests * Only use plugin arrays in form and function tests * Fix spans in function tests * Verify AST in form tests as well * Try to publish @rollup/browser and fix publish-wasm-node-package.js * 4.0.0-12 * Tweak publish scripts * Fix importing wasm file in browser * Parse code as unknown module type for greater compatibility * Remove acorn options * Tweak getReadStringFunction * Fix browser tests * 4.0.0-13 * Remove polyfills that are no longer needed in browser tests * Convert to new import attributes AST format * Rename assertions to attributes * Deprecate externalImportAssertions in favor of externalImportAttributes * Update SWC version * Remove max-call-stack test SWC is not capable of handling it and we cannot fix it * Improve coverage * re-enable repl-artefacts workflow * Fix test * Preload wasm file in docs * docs: add functions to get full path of url * Delete the build plugin handleImportMetaUrl * Make 'npm install github/branch' work * Verify there is a valid changelog entry before releasing * Create release notes and comments from CI Minor change to maybe trigger a CI run * Fix RegExp * 4.0.0-14 * Minor changes for a test PR (#5139) * Fix RegExp use * 4.0.0-15 * Do not rely on current branch to find the PR * 4.0.0-16 * Fix how to determine git commit range * 4.0.0-17 * Make sure we fetch all history on publish * 4.0.0-18 * Add proper permissions * 4.0.0-19 * Use Double quotation marks instead of Single quotation marks if concurrently scripts with flags * Remove "engines" from native packages * Update CONTRIBUTING.md --------- Co-authored-by: XiaoPi <530257315@qq.com> * Update tests * Retrieve the code that was omitted during the merge * Restore test coverage * Update docs/plugin-development/index.md Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com> --------- Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com> Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>
rollup · Sep 18, 2023 · dba42cd · dba42cd
1 parent 35b7ae6
commit dba42cd
Show file tree

Hide file tree

Showing 16 changed files with 53 additions and 42 deletions.
diff --git a/docs/plugin-development/index.md b/docs/plugin-development/index.md
@@ -508,12 +508,8 @@ function injectPolyfillPlugin() {
 				return { id: POLYFILL_ID, moduleSideEffects: true };
 			}
 			if (options.isEntry) {
-				// Determine what the actual entry would have been. We need
-				// "skipSelf" to avoid an infinite loop.
-				const resolution = await this.resolve(source, importer, {
-					skipSelf: true,
-					...options
-				});
+				// Determine what the actual entry would have been.
+				const resolution = await this.resolve(source, importer, options);
 				// If it cannot be resolved or is external, just return it
 				// so that Rollup can display an error
 				if (!resolution || resolution.external) return resolution;
@@ -1536,10 +1532,7 @@ export default function addProxyPlugin() {
 			}
 			// We make sure to pass on any resolveId options to
 			// this.resolve to get the module id
-			const resolution = await this.resolve(source, importer, {
-				skipSelf: true,
-				...options
-			});
+			const resolution = await this.resolve(source, importer, options);
 			// We can only pre-load existing and non-external ids
 			if (resolution && !resolution.external) {
 				// we pass on the entire resolution information
@@ -1699,7 +1692,7 @@ The return type **ResolvedId** of this hook is defined in [`this.getModuleInfo`]
 
 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 an absolute external id is returned that should remain absolute in the output either via the [`makeAbsoluteExternalsRelative`](../configuration-options/index.md#makeabsoluteexternalsrelative) option or by explicit plugin choice in the [`resolveId`](#resolveid) hook, `external` will be `"absolute"` instead of `true`.
 
-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.
+The default of `skipSelf` is `true`, So 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, set `skipSelf` to `false` and 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](#custom-resolver-options) for details.
 

diff --git a/src/utils/PluginContext.ts b/src/utils/PluginContext.ts
@@ -100,6 +100,7 @@ export function getPluginContext(
 		},
 		parse: graph.contextParse.bind(graph),
 		resolve(source, importer, { attributes, custom, isEntry, skipSelf } = BLANK) {
+			skipSelf ??= true;
 			return graph.moduleLoader.resolveId(
 				source,
 				importer,

diff --git a/src/utils/resolveIdViaPlugins.ts b/src/utils/resolveIdViaPlugins.ts
@@ -24,15 +24,17 @@ export function resolveIdViaPlugins(
 		}
 		replaceContext = (pluginContext, plugin): PluginContext => ({
 			...pluginContext,
-			resolve: (source, importer, { attributes, custom, isEntry, skipSelf } = BLANK) =>
-				moduleLoaderResolveId(
+			resolve: (source, importer, { attributes, custom, isEntry, skipSelf } = BLANK) => {
+				skipSelf ??= true;
+				return moduleLoaderResolveId(
 					source,
 					importer,
 					custom,
 					isEntry,
 					attributes || EMPTY_OBJECT,
 					skipSelf ? [...skip, { importer, plugin, source }] : skip
-				)
+				);
+			}
 		});
 	}
 	return pluginDriver.hookFirstAndGetPlugin(

diff --git a/test/chunking-form/samples/preserve-modules-root/_config.js b/test/chunking-form/samples/preserve-modules-root/_config.js
@@ -11,7 +11,7 @@ module.exports = defineTest({
 				name: 'convert-slashes',
 				// This simulates converted slashes as used by e.g. Vite
 				async resolveId(source, importer, options) {
-					const resolved = await this.resolve(source, importer, { ...options, skipSelf: true });
+					const resolved = await this.resolve(source, importer, options);
 					return { ...resolved, id: resolved.id.replace(/\\/g, '/') };
 				}
 			},

diff --git a/test/form/samples/make-absolute-externals-relative/make-relative-false/_config.js b/test/form/samples/make-absolute-externals-relative/make-relative-false/_config.js
@@ -25,7 +25,7 @@ module.exports = defineTest({
 					// eslint-disable-next-line unicorn/consistent-function-scoping
 					const testExternal = async (source, expected) =>
 						assert.deepStrictEqual(
-							(await this.resolve(source, ID_MAIN)).external,
+							(await this.resolve(source, ID_MAIN, { skipSelf: false })).external,
 							expected,
 							source
 						);

diff --git a/test/form/samples/make-absolute-externals-relative/make-relative-relative/_config.js b/test/form/samples/make-absolute-externals-relative/make-relative-relative/_config.js
@@ -28,7 +28,7 @@ module.exports = defineTest({
 					// eslint-disable-next-line unicorn/consistent-function-scoping
 					const testExternal = async (source, expected) =>
 						assert.deepStrictEqual(
-							(await this.resolve(source, ID_MAIN)).external,
+							(await this.resolve(source, ID_MAIN, { skipSelf: false })).external,
 							expected,
 							source
 						);

diff --git a/test/form/samples/make-absolute-externals-relative/make-relative-true/_config.js b/test/form/samples/make-absolute-externals-relative/make-relative-true/_config.js
@@ -27,7 +27,7 @@ module.exports = defineTest({
 					// eslint-disable-next-line unicorn/consistent-function-scoping
 					const testExternal = async (source, expected) =>
 						assert.deepStrictEqual(
-							(await this.resolve(source, ID_MAIN)).external,
+							(await this.resolve(source, ID_MAIN, { skipSelf: false })).external,
 							expected,
 							source
 						);

diff --git a/test/function/samples/context-resolve-skipself/_config.js b/test/function/samples/context-resolve-skipself/_config.js
@@ -1,5 +1,8 @@
 const path = require('node:path');
 
+let hasInfinite = false;
+const importer = path.join(__dirname, 'main.js');
+
 module.exports = defineTest({
 	description: 'allows a plugin to skip its own resolveId hook when using this.resolve',
 	options: {
@@ -9,15 +12,15 @@ module.exports = defineTest({
 					if (id === 'resolutions') {
 						return id;
 					}
-					if (id.startsWith('test')) {
+					if (id.startsWith('test') || id.startsWith('foo') || id.startsWith('bar')) {
 						return 'own-resolution';
 					}
 				},
 				load(id) {
 					if (id === 'resolutions') {
-						const importer = path.join(__dirname, 'main.js');
 						return Promise.all([
-							this.resolve('test', importer).then(result => ({ id: result.id, text: 'all' })),
+							this.resolve('bar', importer).then(result => ({ id: result.id, text: 'bar' })),
+							this.resolve('foo', importer).then(result => ({ id: result.id, text: 'foo' })),
 							this.resolve('test', importer, { skipSelf: false }).then(result => ({
 								id: result.id,
 								text: 'unskipped'
@@ -32,10 +35,27 @@ module.exports = defineTest({
 			},
 			{
 				resolveId(id) {
+					if (id.startsWith('bar')) {
+						return this.resolve('bar', importer);
+					}
+					if (id.startsWith('foo')) {
+						if (hasInfinite) {
+							return 'foo-resolution';
+						}
+						hasInfinite = true;
+						return this.resolve('foo', importer, { skipSelf: false });
+					}
 					if (id.startsWith('test')) {
 						return 'other-resolution';
 					}
 				}
+			},
+			{
+				resolveId(id) {
+					if (id.startsWith('bar')) {
+						return 'bar-resolution';
+					}
+				}
 			}
 		]
 	}

diff --git a/test/function/samples/context-resolve-skipself/existing.js b/test/function/samples/context-resolve-skipself/existing.js
diff --git a/test/function/samples/context-resolve-skipself/main.js b/test/function/samples/context-resolve-skipself/main.js
@@ -2,8 +2,12 @@ import resolutions from 'resolutions';
 
 assert.deepStrictEqual(resolutions, [
 	{
-		id: 'own-resolution',
-		text: 'all'
+		id: 'bar-resolution',
+		text: 'bar'
+	},
+	{
+		id: 'foo-resolution',
+		text: 'foo'
 	},
 	{
 		id: 'own-resolution',

diff --git a/test/function/samples/import-assertions/plugin-assertions-this-resolve/_config.js b/test/function/samples/import-assertions/plugin-assertions-this-resolve/_config.js
@@ -9,7 +9,6 @@ module.exports = defineTest({
 				async resolveId() {
 					assert.deepStrictEqual(
 						await this.resolve('external', undefined, {
-							skipSelf: true,
 							attributes: { a: 'c', b: 'd' }
 						}),
 						{
@@ -28,7 +27,7 @@ module.exports = defineTest({
 				name: 'second',
 				async resolveId(source, importer, { attributes }) {
 					if (source === 'external') {
-						return this.resolve(source, importer, { attributes, skipSelf: true });
+						return this.resolve(source, importer, { attributes });
 					}
 				}
 			},

diff --git a/test/function/samples/module-side-effects/writable/_config.js b/test/function/samples/module-side-effects/writable/_config.js
@@ -18,9 +18,8 @@ module.exports = defineTest({
 						return { id: POLYFILL_ID, moduleSideEffects: true };
 					}
 					if (options.isEntry) {
-						// Determine what the actual entry would have been. We need
-						// "skipSelf" to avoid an infinite loop.
-						const resolution = await this.resolve(source, importer, { skipSelf: true, ...options });
+						// Determine what the actual entry would have been.
+						const resolution = await this.resolve(source, importer, options);
 						// If it cannot be resolved or is external, just return it so that
 						// Rollup can display an error
 						if (!resolution || resolution.external) return resolution;

diff --git a/test/function/samples/preload-cyclic-module/_config.js b/test/function/samples/preload-cyclic-module/_config.js
@@ -18,7 +18,7 @@ module.exports = defineTest({
 					if (!importer || importer.endsWith('?proxy')) {
 						return null;
 					}
-					const resolution = await this.resolve(source, importer, { skipSelf: true, ...options });
+					const resolution = await this.resolve(source, importer, options);
 					if (resolution && !resolution.external) {
 						const moduleInfo = await this.load(resolution);
 						if (moduleInfo.code.includes('/* use proxy */')) {

diff --git a/test/function/samples/preload-module/_config.js b/test/function/samples/preload-module/_config.js
@@ -25,7 +25,7 @@ module.exports = defineTest({
 				},
 				async resolveId(source, importer, options) {
 					if (source.endsWith('main.js')) {
-						const resolvedId = await this.resolve(source, importer, { skipSelf: true, ...options });
+						const resolvedId = await this.resolve(source, importer, options);
 						const { ast, ...moduleInfo } = await this.load({
 							...resolvedId,
 							meta: { testPlugin: 'first' }

diff --git a/test/function/samples/prevent-context-resolve-loop/_config.js b/test/function/samples/prevent-context-resolve-loop/_config.js
@@ -15,7 +15,7 @@ module.exports = defineTest({
 			{
 				name: 'first',
 				async resolveId(source, importer) {
-					const { id } = await this.resolve(source, importer, { skipSelf: true });
+					const { id } = await this.resolve(source, importer);
 					if (id === ID_OTHER_1) {
 						return ID_OTHER_4;
 					}
@@ -24,19 +24,13 @@ module.exports = defineTest({
 			{
 				name: 'second',
 				async resolveId(source, importer) {
-					const { id } = await this.resolve(source, importer, { skipSelf: true });
+					const { id } = await this.resolve(source, importer);
 					if (id === ID_OTHER_2) {
 						// To make this more interesting
 						// The first plugin should resolve everything to 4
-						assert.strictEqual(
-							(await this.resolve('./other1', importer, { skipSelf: true })).id,
-							ID_OTHER_4
-						);
+						assert.strictEqual((await this.resolve('./other1', importer)).id, ID_OTHER_4);
 						// The second file should however be resolved by core as this plugin is out of the loop
-						assert.strictEqual(
-							(await this.resolve(source, ID_OTHER_1, { skipSelf: true })).id,
-							ID_OTHER_2
-						);
+						assert.strictEqual((await this.resolve(source, ID_OTHER_1)).id, ID_OTHER_2);
 						return ID_OTHER_4;
 					}
 				}

diff --git a/test/function/samples/reuse-resolve-meta/_config.js b/test/function/samples/reuse-resolve-meta/_config.js
@@ -10,7 +10,7 @@ module.exports = defineTest({
 		plugins: [
 			{
 				async resolveId(source, importer) {
-					const { id } = await this.resolve(source, importer, { skipSelf: true });
+					const { id } = await this.resolve(source, importer);
 					return { id, meta };
 				},
 				transform(code) {