From b393bcb6fe600e27a33b643a20340dc5669d1f85 Mon Sep 17 00:00:00 2001 From: Tom Jenkinson Date: Sat, 24 Jul 2021 23:04:50 +0100 Subject: [PATCH] fix(node-resolve): Fix bug where JS import was converted to a TS import, resulting in an error when using export maps (#921) --- packages/node-resolve/src/fs.js | 4 + packages/node-resolve/src/index.js | 20 +- .../src/package/resolvePackageImports.js | 2 +- packages/node-resolve/src/package/utils.js | 4 +- .../src/resolveImportSpecifiers.js | 247 ++++++++++++------ .../exports-error-no-fallback/main.js | 5 + .../node_modules/dependency/a.js | 1 + .../node_modules/dependency/package.json | 5 + .../node_modules/dep/index.js | 1 + .../node_modules/dep/package.json | 3 + .../package-json-in-path/package.json/main.js | 5 +- .../import-js-with-js-extension.ts | 3 + .../dependency/dist-esm/something.js | 0 .../dependency/dist-esm/something.ts | 3 + .../node_modules/dependency/package.json | 0 .../node-resolve/test/package-entry-points.js | 2 +- packages/node-resolve/test/test.js | 36 +++ 17 files changed, 242 insertions(+), 99 deletions(-) create mode 100644 packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/main.js create mode 100644 packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/a.js create mode 100644 packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/package.json create mode 100644 packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/index.js create mode 100644 packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/package.json create mode 100644 packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/import-js-with-js-extension.ts rename packages/node-resolve/test/fixtures/{package-json-in-path/package.json => ts-import-js-extension-for-js-file-export-map}/node_modules/dependency/dist-esm/something.js (100%) create mode 100644 packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/dist-esm/something.ts rename packages/node-resolve/test/fixtures/{package-json-in-path/package.json => ts-import-js-extension-for-js-file-export-map}/node_modules/dependency/package.json (100%) diff --git a/packages/node-resolve/src/fs.js b/packages/node-resolve/src/fs.js index 3b780c2ec..f312e1c61 100644 --- a/packages/node-resolve/src/fs.js +++ b/packages/node-resolve/src/fs.js @@ -16,3 +16,7 @@ export async function fileExists(filePath) { return false; } } + +export async function resolveSymlink(path) { + return (await fileExists(path)) ? realpath(path) : path; +} diff --git a/packages/node-resolve/src/index.js b/packages/node-resolve/src/index.js index a8ba3da29..473e590e4 100644 --- a/packages/node-resolve/src/index.js +++ b/packages/node-resolve/src/index.js @@ -121,30 +121,17 @@ export function nodeResolve(opts = {}) { return false; } - const importSpecifierList = []; + const importSpecifierList = [importee]; if (importer === undefined && !importee[0].match(/^\.?\.?\//)) { // For module graph roots (i.e. when importer is undefined), we // need to handle 'path fragments` like `foo/bar` that are commonly // found in rollup config files. If importee doesn't look like a // relative or absolute path, we make it relative and attempt to - // resolve it. If we don't find anything, we try resolving it as we - // got it. + // resolve it. importSpecifierList.push(`./${importee}`); } - const importeeIsBuiltin = builtins.has(importee); - - if (importeeIsBuiltin) { - // The `resolve` library will not resolve packages with the same - // name as a node built-in module. If we're resolving something - // that's a builtin, and we don't prefer to find built-ins, we - // first try to look up a local module with that name. If we don't - // find anything, we resolve the builtin which just returns back - // the built-in's name. - importSpecifierList.push(`${importee}/`); - } - // TypeScript files may import '.js' to refer to either '.ts' or '.tsx' if (importer && importee.endsWith('.js')) { for (const ext of ['.ts', '.tsx']) { @@ -154,8 +141,6 @@ export function nodeResolve(opts = {}) { } } - importSpecifierList.push(importee); - const warn = (...args) => context.warn(...args); const isRequire = opts && opts.custom && opts.custom['node-resolve'] && opts.custom['node-resolve'].isRequire; @@ -180,6 +165,7 @@ export function nodeResolve(opts = {}) { ignoreSideEffectsForRoot }); + const importeeIsBuiltin = builtins.has(importee); const resolved = importeeIsBuiltin && preferBuiltins ? { diff --git a/packages/node-resolve/src/package/resolvePackageImports.js b/packages/node-resolve/src/package/resolvePackageImports.js index 6c180d3ea..e4d405d0e 100644 --- a/packages/node-resolve/src/package/resolvePackageImports.js +++ b/packages/node-resolve/src/package/resolvePackageImports.js @@ -33,7 +33,7 @@ async function resolvePackageImports({ } if (importSpecifier === '#' || importSpecifier.startsWith('#/')) { - throw new InvalidModuleSpecifierError(context, 'Invalid import specifier.'); + throw new InvalidModuleSpecifierError(context, true, 'Invalid import specifier.'); } return resolvePackageImportsExports(context, { diff --git a/packages/node-resolve/src/package/utils.js b/packages/node-resolve/src/package/utils.js index 424bdeb56..31efc8d1e 100644 --- a/packages/node-resolve/src/package/utils.js +++ b/packages/node-resolve/src/package/utils.js @@ -64,8 +64,8 @@ export class InvalidConfigurationError extends ResolveError { } export class InvalidModuleSpecifierError extends ResolveError { - constructor(context, internal) { - super(createErrorMsg(context, internal)); + constructor(context, internal, reason) { + super(createErrorMsg(context, reason, internal)); } } diff --git a/packages/node-resolve/src/resolveImportSpecifiers.js b/packages/node-resolve/src/resolveImportSpecifiers.js index d133e81a1..e0a5b0466 100644 --- a/packages/node-resolve/src/resolveImportSpecifiers.js +++ b/packages/node-resolve/src/resolveImportSpecifiers.js @@ -7,7 +7,7 @@ import { dirname } from 'path'; import resolve from 'resolve'; import { getPackageInfo, getPackageName } from './util'; -import { fileExists, realpath } from './fs'; +import { resolveSymlink } from './fs'; import { isDirCached, isFileCached, readCachedFile } from './cache'; import resolvePackageExports from './package/resolvePackageExports'; import resolvePackageImports from './package/resolvePackageImports'; @@ -34,11 +34,8 @@ async function getPackageJson(importer, pkgName, resolveOptions, moduleDirectori } } -async function resolveId({ - importer, +async function resolveIdClassic({ importSpecifier, - exportConditions, - warn, packageInfoCache, extensions, mainFields, @@ -85,8 +82,38 @@ async function resolveId({ }; let location; + try { + location = await resolveImportPath(importSpecifier, resolveOptions); + } catch (error) { + if (error.code !== 'MODULE_NOT_FOUND') { + throw error; + } + return null; + } - const pkgName = getPackageName(importSpecifier); + return { + location: preserveSymlinks ? location : await resolveSymlink(location), + hasModuleSideEffects, + hasPackageEntry, + packageBrowserField, + packageInfo + }; +} + +async function resolveWithExportMap({ + importer, + importSpecifier, + exportConditions, + packageInfoCache, + extensions, + mainFields, + preserveSymlinks, + useBrowserOverrides, + baseDir, + moduleDirectories, + rootDir, + ignoreSideEffectsForRoot +}) { if (importSpecifier.startsWith('#')) { // this is a package internal import, resolve using package imports field const resolveResult = await resolvePackageImports({ @@ -94,12 +121,9 @@ async function resolveId({ importer, moduleDirs: moduleDirectories, conditions: exportConditions, - resolveId(id, parent) { - return resolveId({ + resolveId(id /* , parent*/) { + return resolveIdClassic({ importSpecifier: id, - importer: parent, - exportConditions, - warn, packageInfoCache, extensions, mainFields, @@ -110,71 +134,91 @@ async function resolveId({ }); } }); - location = fileURLToPath(resolveResult); - } else if (pkgName) { + + const location = fileURLToPath(resolveResult); + return { + location: preserveSymlinks ? location : await resolveSymlink(location), + hasModuleSideEffects: () => null, + hasPackageEntry: true, + packageBrowserField: false, + // eslint-disable-next-line no-undefined + packageInfo: undefined + }; + } + + const pkgName = getPackageName(importSpecifier); + if (pkgName) { // it's a bare import, find the package.json and resolve using package exports if available + let hasModuleSideEffects = () => null; + let hasPackageEntry = true; + let packageBrowserField = false; + let packageInfo; + + const filter = (pkg, pkgPath) => { + const info = getPackageInfo({ + cache: packageInfoCache, + extensions, + pkg, + pkgPath, + mainFields, + preserveSymlinks, + useBrowserOverrides, + rootDir, + ignoreSideEffectsForRoot + }); + + ({ packageInfo, hasModuleSideEffects, hasPackageEntry, packageBrowserField } = info); + + return info.cachedPkg; + }; + + const resolveOptions = { + basedir: baseDir, + readFile: readCachedFile, + isFile: isFileCached, + isDirectory: isDirCached, + extensions, + includeCoreModules: false, + moduleDirectory: moduleDirectories, + preserveSymlinks, + packageFilter: filter + }; + const result = await getPackageJson(importer, pkgName, resolveOptions, moduleDirectories); if (result && result.pkgJson.exports) { - const { pkgJson, pkgJsonPath, pkgPath } = result; - try { - const subpath = - pkgName === importSpecifier ? '.' : `.${importSpecifier.substring(pkgName.length)}`; - const pkgURL = pathToFileURL(`${pkgPath}/`); - - const context = { - importer, - importSpecifier, - moduleDirs: moduleDirectories, - pkgURL, - pkgJsonPath, - conditions: exportConditions - }; - const resolvedPackageExport = await resolvePackageExports( - context, - subpath, - pkgJson.exports - ); - location = fileURLToPath(resolvedPackageExport); - } catch (error) { - if (error instanceof ResolveError) { - return error; - } - throw error; - } - } - } + const { pkgJson, pkgJsonPath } = result; + const subpath = + pkgName === importSpecifier ? '.' : `.${importSpecifier.substring(pkgName.length)}`; + const pkgDr = pkgJsonPath.replace('package.json', ''); + const pkgURL = pathToFileURL(pkgDr); - if (!location) { - // package has no imports or exports, use classic node resolve - try { - location = await resolveImportPath(importSpecifier, resolveOptions); - } catch (error) { - if (error.code !== 'MODULE_NOT_FOUND') { - throw error; + const context = { + importer, + importSpecifier, + moduleDirs: moduleDirectories, + pkgURL, + pkgJsonPath, + conditions: exportConditions + }; + const resolvedPackageExport = await resolvePackageExports(context, subpath, pkgJson.exports); + const location = fileURLToPath(resolvedPackageExport); + if (location) { + return { + location: preserveSymlinks ? location : await resolveSymlink(location), + hasModuleSideEffects, + hasPackageEntry, + packageBrowserField, + packageInfo + }; } - return null; } } - if (!preserveSymlinks) { - if (await fileExists(location)) { - location = await realpath(location); - } - } - - return { - location, - hasModuleSideEffects, - hasPackageEntry, - packageBrowserField, - packageInfo - }; + return null; } -// Resolve module specifiers in order. Promise resolves to the first module that resolves -// successfully, or the error that resulted from the last attempted module resolution. -export default async function resolveImportSpecifiers({ +async function resolveWithClassic({ importer, importSpecifierList, exportConditions, @@ -189,11 +233,9 @@ export default async function resolveImportSpecifiers({ rootDir, ignoreSideEffectsForRoot }) { - let lastResolveError; - for (let i = 0; i < importSpecifierList.length; i++) { // eslint-disable-next-line no-await-in-loop - const result = await resolveId({ + const result = await resolveIdClassic({ importer, importSpecifier: importSpecifierList[i], exportConditions, @@ -209,16 +251,71 @@ export default async function resolveImportSpecifiers({ ignoreSideEffectsForRoot }); - if (result instanceof ResolveError) { - lastResolveError = result; - } else if (result) { + if (result) { return result; } } - if (lastResolveError) { - // only log the last failed resolve error - warn(lastResolveError); - } return null; } + +// Resolves to the module if found or `null`. +// The first import specificer will first be attempted with the exports algorithm. +// If this is unsuccesful because export maps are not being used, then all of `importSpecifierList` +// will be tried with the classic resolution algorithm +export default async function resolveImportSpecifiers({ + importer, + importSpecifierList, + exportConditions, + warn, + packageInfoCache, + extensions, + mainFields, + preserveSymlinks, + useBrowserOverrides, + baseDir, + moduleDirectories, + rootDir, + ignoreSideEffectsForRoot +}) { + try { + const exportMapRes = await resolveWithExportMap({ + importer, + importSpecifier: importSpecifierList[0], + exportConditions, + packageInfoCache, + extensions, + mainFields, + preserveSymlinks, + useBrowserOverrides, + baseDir, + moduleDirectories, + rootDir, + ignoreSideEffectsForRoot + }); + if (exportMapRes) return exportMapRes; + } catch (error) { + if (error instanceof ResolveError) { + warn(error); + return null; + } + throw error; + } + + // package has no imports or exports, use classic node resolve + return resolveWithClassic({ + importer, + importSpecifierList, + exportConditions, + warn, + packageInfoCache, + extensions, + mainFields, + preserveSymlinks, + useBrowserOverrides, + baseDir, + moduleDirectories, + rootDir, + ignoreSideEffectsForRoot + }); +} diff --git a/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/main.js b/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/main.js new file mode 100644 index 000000000..625ffa619 --- /dev/null +++ b/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/main.js @@ -0,0 +1,5 @@ +// should become a-remapped.js, which will fail, +// and then we should not fallback to standard resolve and find the real a.js +import a from 'dependency/a.js'; + +export default a; diff --git a/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/a.js b/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/a.js new file mode 100644 index 000000000..e005d4d66 --- /dev/null +++ b/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/a.js @@ -0,0 +1 @@ +export default 'a'; \ No newline at end of file diff --git a/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/package.json b/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/package.json new file mode 100644 index 000000000..78a8dbd9a --- /dev/null +++ b/packages/node-resolve/test/fixtures/node_modules/exports-error-no-fallback/node_modules/dependency/package.json @@ -0,0 +1,5 @@ +{ + "exports": { + "./a.js": "./a-remapped.js" + } +} diff --git a/packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/index.js b/packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/index.js new file mode 100644 index 000000000..fed149cbd --- /dev/null +++ b/packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/index.js @@ -0,0 +1 @@ +export const test = "It works!" diff --git a/packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/package.json b/packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/package.json new file mode 100644 index 000000000..d814501e5 --- /dev/null +++ b/packages/node-resolve/test/fixtures/package-json-in-path/node_modules/dep/package.json @@ -0,0 +1,3 @@ +{ + "name": "dep" +} diff --git a/packages/node-resolve/test/fixtures/package-json-in-path/package.json/main.js b/packages/node-resolve/test/fixtures/package-json-in-path/package.json/main.js index fb2d93352..4f387b6e4 100644 --- a/packages/node-resolve/test/fixtures/package-json-in-path/package.json/main.js +++ b/packages/node-resolve/test/fixtures/package-json-in-path/package.json/main.js @@ -1,3 +1,2 @@ -import { main } from "dependency/dist/something.js"; - -export default main(); +import { test } from 'dep'; +export default test diff --git a/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/import-js-with-js-extension.ts b/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/import-js-with-js-extension.ts new file mode 100644 index 000000000..fb2d93352 --- /dev/null +++ b/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/import-js-with-js-extension.ts @@ -0,0 +1,3 @@ +import { main } from "dependency/dist/something.js"; + +export default main(); diff --git a/packages/node-resolve/test/fixtures/package-json-in-path/package.json/node_modules/dependency/dist-esm/something.js b/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/dist-esm/something.js similarity index 100% rename from packages/node-resolve/test/fixtures/package-json-in-path/package.json/node_modules/dependency/dist-esm/something.js rename to packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/dist-esm/something.js diff --git a/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/dist-esm/something.ts b/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/dist-esm/something.ts new file mode 100644 index 000000000..bc23cef80 --- /dev/null +++ b/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/dist-esm/something.ts @@ -0,0 +1,3 @@ +export function main() { + return "Wrong file!"; +} diff --git a/packages/node-resolve/test/fixtures/package-json-in-path/package.json/node_modules/dependency/package.json b/packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/package.json similarity index 100% rename from packages/node-resolve/test/fixtures/package-json-in-path/package.json/node_modules/dependency/package.json rename to packages/node-resolve/test/fixtures/ts-import-js-extension-for-js-file-export-map/node_modules/dependency/package.json diff --git a/packages/node-resolve/test/package-entry-points.js b/packages/node-resolve/test/package-entry-points.js index cb2b38a3f..46c5a9e2a 100644 --- a/packages/node-resolve/test/package-entry-points.js +++ b/packages/node-resolve/test/package-entry-points.js @@ -352,7 +352,7 @@ test('can override a star pattern using null', async (t) => { test('can self-import a package when using exports field', async (t) => { const bundle = await rollup({ - input: 'self-package-import', + input: 'self-package-import.js', onwarn: () => { t.fail('No warnings were expected'); }, diff --git a/packages/node-resolve/test/test.js b/packages/node-resolve/test/test.js index b47056fce..0dcfb5e5e 100755 --- a/packages/node-resolve/test/test.js +++ b/packages/node-resolve/test/test.js @@ -105,6 +105,23 @@ test('supports non-standard extensions', async (t) => { await testBundle(t, bundle); }); +test('does not fallback to standard node resolve algorithm if error with exports one', async (t) => { + try { + await rollup({ + input: 'exports-error-no-fallback/main.js', + onwarn: () => t.fail('No warnings were expected'), + plugins: [ + nodeResolve({ + extensions: ['.js'] + }) + ] + }); + t.fail('expecting throw'); + } catch { + t.pass(); + } +}); + test('supports JS extensions in TS when referring to TS imports', async (t) => { const bundle = await rollup({ input: 'ts-import-js-extension/import-ts-with-js-extension.ts', @@ -124,6 +141,25 @@ test('supports JS extensions in TS when referring to TS imports', async (t) => { t.is(module.exports, 'It works!'); }); +test('supports JS extensions in TS actually importing JS with export map', async (t) => { + const bundle = await rollup({ + input: 'ts-import-js-extension-for-js-file-export-map/import-js-with-js-extension.ts', + onwarn: () => t.fail('No warnings were expected'), + plugins: [ + nodeResolve({ + extensions: ['.js', '.ts'] + }), + babel({ + babelHelpers: 'bundled', + plugins: ['@babel/plugin-transform-typescript'], + extensions: ['.js', '.ts'] + }) + ] + }); + const { module } = await testBundle(t, bundle); + t.is(module.exports, 'It works!'); +}); + test('handles package.json being a directory earlier in the path', async (t) => { const bundle = await rollup({ input: 'package-json-in-path/package.json/main.js',