Properly handle upper directories as external dependencies (#4419)

* Allow external directory imports * Allow external directory imports * Fix plugin name * Test on Windows * Test on Windows
rollup · Mar 2, 2022 · 0b60dd8 · 0b60dd8
1 parent b74cb92
commit 0b60dd8
Show file tree

Hide file tree

Showing 14 changed files with 76 additions and 32 deletions.
diff --git a/build-plugins/esm-dynamic-import.ts b/build-plugins/esm-dynamic-import.ts
@@ -1,6 +1,6 @@
 import type { Plugin } from 'rollup';
 
-export default function addBinShebangAndEsmImport(): Plugin {
+export default function esmDynamicImport(): Plugin {
 	let importFound = false;
 	return {
 		generateBundle() {

diff --git a/src/Chunk.ts b/src/Chunk.ts
@@ -51,8 +51,8 @@ import {
 	isDefaultAProperty,
 	namespaceInteropHelpersByInteropType
 } from './utils/interopHelpers';
-import { basename, dirname, extname, isAbsolute, normalize, resolve } from './utils/path';
-import relativeId, { getAliasName } from './utils/relativeId';
+import { dirname, extname, isAbsolute, normalize, resolve } from './utils/path';
+import relativeId, { getAliasName, getImportPath } from './utils/relativeId';
 import renderChunk from './utils/renderChunk';
 import type { RenderOptions } from './utils/renderHelpers';
 import { makeUnique, renderNamePattern } from './utils/renderNamePattern';
@@ -691,11 +691,13 @@ export default class Chunk {
 			if (dependency instanceof ExternalModule) {
 				const originalId = dependency.renderPath;
 				renderedDependency.id = escapeId(
-					dependency.renormalizeRenderPath ? this.getRelativePath(originalId, false) : originalId
+					dependency.renormalizeRenderPath
+						? getImportPath(this.id!, originalId, false, false)
+						: originalId
 				);
 			} else {
 				renderedDependency.namedExportsMode = dependency.exportMode !== 'default';
-				renderedDependency.id = escapeId(this.getRelativePath(dependency.id!, false));
+				renderedDependency.id = escapeId(getImportPath(this.id!, dependency.id!, false, true));
 			}
 		}
 
@@ -926,12 +928,12 @@ export default class Chunk {
 				const renderedResolution =
 					resolution instanceof Module
 						? `'${escapeId(
-								this.getRelativePath((facadeChunk || chunk!).id!, stripKnownJsExtensions)
+								getImportPath(this.id!, (facadeChunk || chunk!).id!, stripKnownJsExtensions, true)
 						  )}'`
 						: resolution instanceof ExternalModule
 						? `'${escapeId(
 								resolution.renormalizeRenderPath
-									? this.getRelativePath(resolution.renderPath, stripKnownJsExtensions)
+									? getImportPath(this.id!, resolution.renderPath, stripKnownJsExtensions, false)
 									: resolution.renderPath
 						  )}'`
 						: resolution;
@@ -1212,16 +1214,6 @@ export default class Chunk {
 		return referencedFiles;
 	}
 
-	private getRelativePath(targetPath: string, stripJsExtension: boolean): string {
-		let relativePath = normalize(relative(dirname(this.id!), targetPath));
-		if (stripJsExtension && relativePath.endsWith('.js')) {
-			relativePath = relativePath.slice(0, -3);
-		}
-		if (relativePath === '..') return '../../' + basename(targetPath);
-		if (relativePath === '') return '../' + basename(targetPath);
-		return relativePath.startsWith('../') ? relativePath : './' + relativePath;
-	}
-
 	private inlineChunkDependencies(chunk: Chunk): void {
 		for (const dep of chunk.dependencies) {
 			if (this.dependencies.has(dep)) continue;

diff --git a/src/ExternalModule.ts b/src/ExternalModule.ts
@@ -87,15 +87,14 @@ export default class ExternalModule {
 		return [externalVariable];
 	}
 
-	setRenderPath(options: NormalizedOutputOptions, inputBase: string): string {
+	setRenderPath(options: NormalizedOutputOptions, inputBase: string): void {
 		this.renderPath =
 			typeof options.paths === 'function' ? options.paths(this.id) : options.paths[this.id];
 		if (!this.renderPath) {
 			this.renderPath = this.renormalizeRenderPath
 				? normalize(relative(inputBase, this.id))
 				: this.id;
 		}
-		return this.renderPath;
 	}
 
 	suggestName(name: string): void {

diff --git a/src/utils/path.ts b/src/utils/path.ts
@@ -1,5 +1,5 @@
 const ABSOLUTE_PATH_REGEX = /^(?:\/|(?:[A-Za-z]:)?[\\|/])/;
-const RELATIVE_PATH_REGEX = /^\.?\.\//;
+const RELATIVE_PATH_REGEX = /^\.?\.(\/|$)/;
 
 export function isAbsolute(path: string): boolean {
 	return ABSOLUTE_PATH_REGEX.test(path);

diff --git a/src/utils/relativeId.ts b/src/utils/relativeId.ts
@@ -1,4 +1,5 @@
-import { basename, extname, isAbsolute, relative, resolve } from './path';
+import { relative } from '../../browser/path';
+import { basename, dirname, extname, isAbsolute, normalize, resolve } from './path';
 
 export function getAliasName(id: string): string {
 	const base = basename(id);
@@ -16,3 +17,27 @@ export function isPathFragment(name: string): boolean {
 		name[0] === '/' || (name[0] === '.' && (name[1] === '/' || name[1] === '.')) || isAbsolute(name)
 	);
 }
+
+const UPPER_DIR_REGEX = /^(\.\.\/)*\.\.$/;
+
+export function getImportPath(
+	importerId: string,
+	targetPath: string,
+	stripJsExtension: boolean,
+	ensureFileName: boolean
+): string {
+	let relativePath = normalize(relative(dirname(importerId), targetPath));
+	if (stripJsExtension && relativePath.endsWith('.js')) {
+		relativePath = relativePath.slice(0, -3);
+	}
+	if (ensureFileName) {
+		if (relativePath === '') return '../' + basename(targetPath);
+		if (UPPER_DIR_REGEX.test(relativePath)) {
+			return relativePath
+				.split('/')
+				.concat(['..', basename(targetPath)])
+				.join('/');
+		}
+	}
+	return !relativePath ? '.' : relativePath.startsWith('..') ? relativePath : './' + relativePath;
+}
diff --git a/test/cli/samples/custom-frame/_config.js b/test/cli/samples/custom-frame/_config.js
@@ -9,7 +9,7 @@ module.exports = {
 			stderr,
 			'[!] (plugin at position 1) Error: My error.\n' +
 				'main.js\ncustom code frame\nError: My error.\n' +
-				'    at Object.transform'
+				'    at Object.'
 		);
 		assertIncludes(stderr, 'rollup.config.js:11:17');
 	}

diff --git a/test/cli/samples/watch/no-config-file/_config.js b/test/cli/samples/watch/no-config-file/_config.js
@@ -4,7 +4,7 @@ module.exports = {
 	description: 'watches without a config file',
 	command: 'rollup main.js --watch --format es --file _actual/main.js',
 	abortOnStderr(data) {
-		if (data.includes(`created _actual${path.sep}main.js`)) {
+		if (data.includes(`created _actual/main.js`)) {
 			return true;
 		}
 	}

diff --git a/test/cli/samples/watch/node-config-file/_config.js b/test/cli/samples/watch/node-config-file/_config.js
@@ -4,7 +4,7 @@ module.exports = {
 	description: 'watches using a node_modules config files',
 	command: 'rollup --watch --config node:custom',
 	abortOnStderr(data) {
-		if (data.includes(`created _actual${path.sep}main.js`)) {
+		if (data.includes(`created _actual/main.js`)) {
 			return true;
 		}
 	}

diff --git a/test/cli/samples/watch/watch-config-early-update/_config.js b/test/cli/samples/watch/watch-config-early-update/_config.js
@@ -45,6 +45,7 @@ module.exports = {
 	},
 	after() {
 		unlinkSync(configFile);
+		stopUpdate();
 	},
 	abortOnStderr(data) {
 		if (data === 'initial\n') {
@@ -62,7 +63,7 @@ module.exports = {
 			);
 			return false;
 		}
-		if (data.includes(`created _actual${path.sep}output2.js`)) {
+		if (data.includes(`created _actual/output2.js`)) {
 			stopUpdate();
 			return true;
 		}

diff --git a/test/cli/samples/watch/watch-config-error/_config.js b/test/cli/samples/watch/watch-config-error/_config.js
@@ -26,7 +26,7 @@ module.exports = {
 		setTimeout(() => unlinkSync(configFile), 300);
 	},
 	abortOnStderr(data) {
-		if (data.includes(`created _actual${path.sep}main1.js`)) {
+		if (data.includes(`created _actual/main1.js`)) {
 			setTimeout(
 				() => atomicWriteFileSync(configFile, 'throw new Error("Config contains errors");'),
 				600
@@ -48,7 +48,7 @@ module.exports = {
 			}, 600);
 			return false;
 		}
-		if (data.includes(`created _actual${path.sep}main2.js`)) {
+		if (data.includes(`created _actual/main2.js`)) {
 			return true;
 		}
 	}

diff --git a/test/cli/samples/watch/watch-config-no-update/_config.js b/test/cli/samples/watch/watch-config-no-update/_config.js
@@ -23,7 +23,7 @@ module.exports = {
 		unlinkSync(configFile);
 	},
 	abortOnStderr(data) {
-		if (data.includes(`created _actual${path.sep}main.js`)) {
+		if (data.includes(`created _actual/main.js`)) {
 			atomicWriteFileSync(configFile, configContent);
 			// wait some time for the watcher to trigger
 			return new Promise(resolve => setTimeout(() => resolve(true), 600));

diff --git a/test/function/samples/external-directory-import/_config.js b/test/function/samples/external-directory-import/_config.js
@@ -0,0 +1,11 @@
+module.exports = {
+	description: 'handles using ../ as external import (#4349)',
+	options: {
+		external() {
+			return true;
+		}
+	},
+	context: {
+		require: id => id
+	}
+};
diff --git a/test/function/samples/external-directory-import/main.js b/test/function/samples/external-directory-import/main.js
@@ -0,0 +1,13 @@
+import foo1 from './';
+import foo2 from '../';
+import foo3 from '.';
+import foo4 from '..';
+import foo5 from './index.js';
+import foo6 from '../index.js';
+
+assert.strictEqual(foo1, '.');
+assert.strictEqual(foo2, '..');
+assert.strictEqual(foo3, '.');
+assert.strictEqual(foo4, '..');
+assert.strictEqual(foo5, './index.js');
+assert.strictEqual(foo6, '../index.js');
diff --git a/test/misc/misc.js b/test/misc/misc.js
@@ -221,29 +221,32 @@ console.log(x);
 			input: {
 				'base/main': 'main.js',
 				'base/main/feature': 'feature.js',
-				'base/main/feature/sub': 'subfeature.js'
+				'base/main/feature/sub': 'subfeature.js',
+				'base/main/feature/sub/sub': 'subsubfeature.js'
 			},
 			plugins: [
 				loader({
 					'main.js': 'export function fn () { return "main"; } console.log(fn());',
 					'feature.js': 'import { fn } from "main.js"; console.log(fn() + " feature");',
-					'subfeature.js': 'import { fn } from "main.js"; console.log(fn() + " subfeature");'
+					'subfeature.js': 'import { fn } from "main.js"; console.log(fn() + " subfeature");',
+					'subsubfeature.js': 'import { fn } from "main.js"; console.log(fn() + " subsubfeature");'
 				})
 			]
 		});
 		const {
-			output: [main, feature, subfeature]
+			output: [main, feature, subfeature, subsubfeature]
 		} = await bundle.generate({
 			entryFileNames: `[name]`,
 			chunkFileNames: `[name]`,
 			format: 'es'
 		});
 		assert.strictEqual(main.fileName, 'base/main');
-		assert.ok(main.code.startsWith('function fn'));
 		assert.strictEqual(feature.fileName, 'base/main/feature');
 		assert.ok(feature.code.startsWith("import { fn } from '../main'"));
 		assert.strictEqual(subfeature.fileName, 'base/main/feature/sub');
 		assert.ok(subfeature.code.startsWith("import { fn } from '../../main'"));
+		assert.strictEqual(subsubfeature.fileName, 'base/main/feature/sub/sub');
+		assert.ok(subsubfeature.code.startsWith("import { fn } from '../../../main'"));
 	});
 
 	it('throws the proper error on max call stack exception', async () => {