fix: remove isolated resolve() for compat with browser distribution (#…

…3935)

* fix: remove isolated resolve()

This is incompatible with the browser implementation of the method.

* Set up basic browser tests

* Update test suites

* Update browser/error.ts

Co-authored-by: Craig Morten <cmorten@users.noreply.github.com>

* Use posix paths in test config

Co-authored-by: Lukas Taegert-Atkinson <lukas.taegert-atkinson@tngtech.com>
Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>

.nycrc

@@ -1,4 +1,4 @@
 {
-  "exclude": ["*commonjsHelpers.js", "test"],
+  "exclude": ["test"],
   "extension": [".ts", ""]
 }

.vscode/launch.json

@@ -1,7 +1,4 @@
 {
-    // Use IntelliSense to learn about possible attributes.
-    // Hover to view descriptions of existing attributes.
-    // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
     "version": "0.2.0",
     "configurations": [
         {

.vscode/settings.json

@@ -1,4 +1,3 @@
-// Place your settings in this file to overwrite default and user settings.
 {
   "typescript.format.insertSpaceBeforeFunctionParenthesis": true,
   "typescript.format.insertSpaceAfterConstructor": true,

browser/error.ts

@@ -0,0 +1,9 @@
+import { error } from '../src/utils/error';
+
+export const throwNoFileSystem = (method: string) => (..._args: any[]): never => {
+	error({
+		code: 'NO_FS_IN_BROWSER',
+		message: `Cannot access the file system (via "${method}") when using the browser build of Rollup. Make sure you supply a plugin with custom resolveId and load hooks to Rollup.`,
+		url: 'https://rollupjs.org/guide/en/#a-simple-example'
+	});
+};

browser/fs.ts

@@ -1,14 +1,4 @@
-const nope = (method: string) => (..._args: any[]): never => {
-	throw Object.assign(
-		new Error(
-			`Cannot access the file system (via "fs.${method}") when using the browser build of Rollup. Make sure you supply a plugin with custom resolveId and load hooks to Rollup.`
-		),
-		{ code: 'NO_FS_IN_BROWSER', url: 'https://rollupjs.org/guide/en/#a-simple-example' }
-	);
-};
+import { throwNoFileSystem } from './error';
 
-export const lstatSync = nope('lstatSync');
-export const readdirSync = nope('readdirSync');
-export const readFile = nope('readFile');
-export const realpathSync = nope('realpathSync');
-export const writeFile = nope('writeFile');
+export const readFile = throwNoFileSystem('fs.readFile');
+export const writeFile = throwNoFileSystem('fs.writeFile');

browser/path.ts

@@ -58,9 +58,13 @@ export function relative(from: string, to: string) {
 }
 
 export function resolve(...paths: string[]) {
-	let resolvedParts = paths.shift()!.split(/[/\\]/);
+	const firstPathSegment = paths.shift();
+	if (!firstPathSegment) {
+		return '/';
+	}
+	let resolvedParts = firstPathSegment.split(/[/\\]/);
 
-	paths.forEach(path => {
+	for (const path of paths) {
 		if (isAbsolute(path)) {
 			resolvedParts = path.split(/[/\\]/);
 		} else {
@@ -75,7 +79,7 @@ export function resolve(...paths: string[]) {
 
 			resolvedParts.push.apply(resolvedParts, parts);
 		}
-	});
+	}
 
 	return resolvedParts.join('/');
 }

browser/resolveId.ts

@@ -0,0 +1,23 @@
+import { CustomPluginOptions } from '../src/rollup/types';
+import { PluginDriver } from '../src/utils/PluginDriver';
+import { throwNoFileSystem } from './error';
+
+export async function resolveId(
+	source: string,
+	importer: string | undefined,
+	_preserveSymlinks: boolean,
+	pluginDriver: PluginDriver,
+	skip: number | null,
+	customOptions: CustomPluginOptions | undefined
+) {
+	const pluginResult = await pluginDriver.hookFirst(
+		'resolveId',
+		[source, importer, { custom: customOptions }],
+		null,
+		skip
+	);
+	if (pluginResult == null) {
+		throwNoFileSystem('path.resolve');
+	}
+	return pluginResult;
+}

build-plugins/replace-browser-modules.js

@@ -0,0 +1,27 @@
+import path from 'path';
+
+const ID_CRYPTO = path.resolve('src/utils/crypto');
+const ID_FS = path.resolve('src/utils/fs');
+const ID_PATH = path.resolve('src/utils/path');
+const ID_RESOLVEID = path.resolve('src/utils/resolveId');
+
+export default function replaceBrowserModules() {
+	return {
+		name: 'replace-browser-modules',
+		resolveId: (source, importee) => {
+			if (importee && source[0] === '.') {
+				const resolved = path.join(path.dirname(importee), source);
+				switch (resolved) {
+					case ID_CRYPTO:
+						return path.resolve('browser/crypto.ts');
+					case ID_FS:
+						return path.resolve('browser/fs.ts');
+					case ID_PATH:
+						return path.resolve('browser/path.ts');
+					case ID_RESOLVEID:
+						return path.resolve('browser/resolveId.ts');
+				}
+			}
+		}
+	};
+}

package.json

@@ -10,34 +10,34 @@
   },
   "scripts": {
     "build": "shx rm -rf dist && git rev-parse HEAD > .commithash && rollup -c && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
-    "build:test": "shx rm -rf dist && rollup -c --configTest && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
+    "build:cjs": "shx rm -rf dist && rollup -c --configTest && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
     "build:bootstrap": "dist/bin/rollup -c && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
     "ci:lint": "npm run lint:nofix",
-    "ci:test": "npm run build:test && npm run build:bootstrap && npm run test:all",
-    "ci:test:only": "npm run build:test && npm run build:bootstrap && npm run test:only",
-    "ci:coverage": "npm run build:test && nyc --reporter lcovonly mocha && codecov",
+    "ci:test": "npm run build:cjs && npm run build:bootstrap && npm run test:all",
+    "ci:test:only": "npm run build:cjs && npm run build:bootstrap && npm run test:only",
+    "ci:coverage": "npm run build:cjs && nyc --reporter lcovonly mocha && codecov",
     "lint": "npm run lint:ts -- --fix && npm run lint:js -- --fix && npm run lint:markdown",
     "lint:nofix": "npm run lint:ts && npm run lint:js && npm run lint:markdown",
     "lint:ts": "tslint --project .",
     "lint:js": "eslint test/test.js test/*/index.js test/utils.js test/**/_config.js",
     "lint:markdown": "markdownlint --config markdownlint.json docs/**/*.md",
-    "perf": "npm run build:test && node --expose-gc scripts/perf.js",
+    "perf": "npm run build:cjs && node --expose-gc scripts/perf.js",
     "perf:debug": "node --inspect-brk scripts/perf-debug.js",
     "perf:init": "node scripts/perf-init.js",
     "prepare": "npm run build",
     "prepublishOnly": "npm ci && npm run lint:nofix && npm run security && npm run build:bootstrap && npm run test:all",
-    "pretest": "npm run build:test",
-    "pretest:coverage": "npm run build:test && shx rm -rf coverage/*",
-    "pretest:typescript": "shx rm -rf test/typescript/dist && shx cp -r dist test/typescript/",
-    "security": "# npm audit # deactivated until there is a solution for the lodash issue",
-    "test": "npm run test:all",
-    "test:all": "npm run test:only && npm run test:typescript && npm run test:leak && npm run test:package",
-    "test:coverage": "nyc --reporter html mocha",
+    "security": "npm audit",
+    "test": "npm run build && npm run test:all",
+    "test:cjs": "npm run build:cjs && npm run test:only",
+    "test:quick": "mocha -b test/test.js",
+    "test:all": "npm run test:only && npm run test:browser && npm run test:typescript && npm run test:leak && npm run test:package",
+    "test:coverage": "npm run build:cjs && shx rm -rf coverage/* && nyc --reporter html mocha test/test.js",
+    "test:coverage:browser": "npm run build && shx rm -rf coverage/* && nyc mocha test/browser/index.js",
     "test:leak": "node --expose-gc test/leak/index.js",
     "test:package": "node scripts/test-package.js",
-    "test:only": "mocha",
-    "test:quick": "mocha -b",
-    "test:typescript": "tsc --noEmit -p test/typescript && tsc --noEmit",
+    "test:only": "mocha test/test.js",
+    "test:typescript": "shx rm -rf test/typescript/dist && shx cp -r dist test/typescript/ && tsc --noEmit -p test/typescript && tsc --noEmit",
+    "test:browser": "mocha test/browser/index.js",
     "watch": "rollup -cw"
   },
   "repository": "rollup/rollup",

rollup.config.js

@@ -12,6 +12,7 @@ import conditionalFsEventsImport from './build-plugins/conditional-fsevents-impo
 import emitModulePackageFile from './build-plugins/emit-module-package-file.js';
 import esmDynamicImport from './build-plugins/esm-dynamic-import.js';
 import getLicenseHandler from './build-plugins/generate-license-file';
+import replaceBrowserModules from './build-plugins/replace-browser-modules.js';
 import pkg from './package.json';
 
 const commitHash = (function () {
@@ -142,16 +143,10 @@ export default command => {
 		input: 'src/browser-entry.ts',
 		onwarn,
 		plugins: [
+			replaceBrowserModules(),
 			alias(moduleAliases),
 			resolve({ browser: true }),
 			json(),
-			{
-				load: id => {
-					if (~id.indexOf('crypto.ts')) return fs.readFileSync('browser/crypto.ts', 'utf-8');
-					if (~id.indexOf('fs.ts')) return fs.readFileSync('browser/fs.ts', 'utf-8');
-					if (~id.indexOf('path.ts')) return fs.readFileSync('browser/path.ts', 'utf-8');
-				}
-			},
 			commonjs(),
 			typescript(),
 			terser({ module: true, output: { comments: 'some' } }),
@@ -161,7 +156,7 @@ export default command => {
 		treeshake,
 		strictDeprecations: true,
 		output: [
-			{ file: 'dist/rollup.browser.js', format: 'umd', name: 'rollup', banner },
+			{ file: 'dist/rollup.browser.js', format: 'umd', name: 'rollup', banner, sourcemap: true },
 			{ file: 'dist/es/rollup.browser.js', format: 'es', banner }
 		]
 	};

src/utils/relativeId.ts

@@ -1,4 +1,4 @@
-import { basename, extname, isAbsolute, relative } from './path';
+import { basename, extname, isAbsolute, relative, resolve } from './path';
 import { sanitizeFileName } from './sanitizeFileName';
 
 export function getAliasName(id: string) {
@@ -7,8 +7,8 @@ export function getAliasName(id: string) {
 }
 
 export default function relativeId(id: string) {
-	if (typeof process === 'undefined' || !isAbsolute(id)) return id;
-	return relative(process.cwd(), id);
+	if (!isAbsolute(id)) return id;
+	return relative(resolve(), id);
 }
 
 export function isPlainPathFragment(name: string) {

src/utils/resolveId.ts

@@ -28,7 +28,7 @@ export async function resolveId(
 	// resolve call and require no special handing on our part.
 	// See https://nodejs.org/api/path.html#path_path_resolve_paths
 	return addJsExtensionIfNecessary(
-		resolve(importer ? dirname(importer) : resolve(), source),
+		importer ? resolve(dirname(importer), source) : resolve(source),
 		preserveSymlinks
 	);
 }

test/browser/index.js

@@ -0,0 +1,76 @@
+const fixturify = require('fixturify');
+const { basename, resolve } = require('path');
+const { rollup } = require('../../dist/rollup.browser.js');
+const { assertFilesAreEqual, runTestSuiteWithSamples, compareError } = require('../utils.js');
+
+runTestSuiteWithSamples('browser', resolve(__dirname, 'samples'), (dir, config) => {
+	(config.skip ? it.skip : config.solo ? it.only : it)(
+		basename(dir) + ': ' + config.description,
+		async () => {
+			let bundle;
+			try {
+				bundle = await rollup({
+					input: 'main',
+					onwarn: warning => {
+						if (!(config.expectedWarnings && config.expectedWarnings.indexOf(warning.code) >= 0)) {
+							throw new Error(
+								`Unexpected warnings (${warning.code}): ${warning.message}\n` +
+									'If you expect warnings, list their codes in config.expectedWarnings'
+							);
+						}
+					},
+					strictDeprecations: true,
+					...config.options
+				});
+			} catch (error) {
+				if (config.error) {
+					compareError(error, config.error);
+					return;
+				} else {
+					throw error;
+				}
+			}
+			if (config.error) {
+				throw new Error('Expected an error while rolling up');
+			}
+			let output;
+			try {
+				({ output } = await bundle.generate({
+					exports: 'auto',
+					format: 'es',
+					...(config.options || {}).output
+				}));
+			} catch (error) {
+				if (config.generateError) {
+					compareError(error, config.generateError);
+					return;
+				} else {
+					throw error;
+				}
+			}
+			if (config.generateError) {
+				throw new Error('Expected an error while generating output');
+			}
+			assertOutputMatches(output, dir);
+		}
+	);
+});
+
+function assertOutputMatches(output, dir) {
+	const actual = {};
+	for (const file of output) {
+		const filePath = file.fileName.split('/');
+		const fileName = filePath.pop();
+		let currentDir = actual;
+		for (const pathElement of filePath) {
+			if (!currentDir[pathElement]) {
+				currentDir[pathElement] = {};
+			}
+			currentDir = currentDir[pathElement] = currentDir[pathElement] || {};
+		}
+		currentDir[fileName] = file.source || file.code;
+	}
+	fixturify.writeSync(resolve(dir, '_actual'), actual);
+	const expected = fixturify.readSync(resolve(dir, '_expected'));
+	assertFilesAreEqual(actual, expected);
+}

test/browser/samples/basic/_config.js

@@ -0,0 +1,12 @@
+const { loader } = require('../../../utils.js');
+
+module.exports = {
+	description: 'bundles files for the browser',
+	options: {
+		plugins: loader({
+			main: `import {foo} from 'dep';
+console.log(foo);`,
+			dep: `export const foo = 42;`
+		})
+	}
+};

test/browser/samples/basic/_expected/main.js

@@ -0,0 +1,3 @@
+const foo = 42;
+
+console.log(foo);

test/browser/samples/missing-dependency-resolution/_config.js

@@ -0,0 +1,16 @@
+const { loader } = require('../../../utils.js');
+
+module.exports = {
+	description: 'fails if a dependency cannot be resolved',
+	options: {
+		plugins: loader({
+			main: `import {foo} from 'dep';
+console.log(foo);`
+		})
+	},
+	error: {
+		message:
+			"Unexpected warnings (UNRESOLVED_IMPORT): 'dep' is imported by main, but could not be resolved – treating it as an external dependency\nIf you expect warnings, list their codes in config.expectedWarnings",
+		watchFiles: ['main']
+	}
+};

test/browser/samples/missing-entry-resolution/_config.js

@@ -0,0 +1,7 @@
+module.exports = {
+	description: 'fails if an entry cannot be resolved',
+	error: {
+		code: 'UNRESOLVED_ENTRY',
+		message: 'Could not resolve entry module (main).'
+	}
+};

test/browser/samples/missing-load/_config.js

@@ -0,0 +1,17 @@
+module.exports = {
+	description: 'fails if a file cannot be loaded',
+	options: {
+		plugins: {
+			resolveId(source) {
+				return source;
+			}
+		}
+	},
+	error: {
+		code: 'NO_FS_IN_BROWSER',
+		message:
+			'Could not load main: Cannot access the file system (via "fs.readFile") when using the browser build of Rollup. Make sure you supply a plugin with custom resolveId and load hooks to Rollup.',
+		url: 'https://rollupjs.org/guide/en/#a-simple-example',
+		watchFiles: ['main']
+	}
+};