From 3df00f4e64fd013ef8bc789b0be0496a033f4d3d Mon Sep 17 00:00:00 2001 From: Hiroki Osame Date: Tue, 30 Jul 2024 22:17:40 +0900 Subject: [PATCH] fix(resolver): prioritize requested path in dependencies fixes privatenumber/tsx#617 --- src/cjs/api/module-extensions.ts | 7 ++- src/cjs/api/module-resolve-filename.ts | 61 ++++++++++++--------- src/cjs/api/resolve-implicit-extensions.ts | 43 +++++++++++---- src/esm/hook/load.ts | 7 ++- src/esm/hook/resolve.ts | 34 +++++++----- src/utils/map-ts-extensions.ts | 63 ++++++++++++++++------ src/utils/path-utils.ts | 6 ++- tests/fixtures.ts | 12 ++++- tests/specs/smoke.ts | 4 ++ 9 files changed, 168 insertions(+), 69 deletions(-) diff --git a/src/cjs/api/module-extensions.ts b/src/cjs/api/module-extensions.ts index 5a7599459..a8c5e69c9 100644 --- a/src/cjs/api/module-extensions.ts +++ b/src/cjs/api/module-extensions.ts @@ -7,7 +7,6 @@ import { isESM } from '../../utils/es-module-lexer.js'; import { shouldApplySourceMap, inlineSourceMap } from '../../source-map.js'; import { parent } from '../../utils/ipc/client.js'; import { fileMatcher } from '../../utils/tsconfig.js'; -import { implicitlyResolvableExtensions } from './resolve-implicit-extensions.js'; import type { LoaderState } from './types.js'; const typescriptExtensions = [ @@ -24,6 +23,12 @@ const transformExtensions = [ '.mjs', ] as const; +const implicitlyResolvableExtensions = [ + '.ts', + '.tsx', + '.jsx', +] as const; + const safeSet = >( object: T, property: keyof T, diff --git a/src/cjs/api/module-resolve-filename.ts b/src/cjs/api/module-resolve-filename.ts index 1ba5a0b31..46e895eea 100644 --- a/src/cjs/api/module-resolve-filename.ts +++ b/src/cjs/api/module-resolve-filename.ts @@ -3,14 +3,19 @@ import Module from 'node:module'; import { fileURLToPath } from 'node:url'; import { mapTsExtensions } from '../../utils/map-ts-extensions.js'; import type { NodeError } from '../../types.js'; -import { isRelativePath, fileUrlPrefix, tsExtensionsPattern } from '../../utils/path-utils.js'; +import { + isRelativePath, + isFilePath, + fileUrlPrefix, + tsExtensionsPattern, + isDirectoryPattern, + nodeModulesPath, +} from '../../utils/path-utils.js'; import { tsconfigPathsMatcher, allowJs } from '../../utils/tsconfig.js'; import { urlSearchParamsStringify } from '../../utils/url-search-params-stringify.js'; import type { ResolveFilename, SimpleResolve, LoaderState } from './types.js'; import { createImplicitResolver } from './resolve-implicit-extensions.js'; -const nodeModulesPath = `${path.sep}node_modules${path.sep}`; - const getOriginalFilePath = ( request: string, ) => { @@ -53,8 +58,11 @@ const resolveTsFilename = ( parent: Module.Parent | undefined, ) => { if ( - !(parent?.filename && tsExtensionsPattern.test(parent.filename)) - && !allowJs + isDirectoryPattern.test(request) + || ( + !(parent?.filename && tsExtensionsPattern.test(parent.filename)) + && !allowJs + ) ) { return; } @@ -82,7 +90,7 @@ const resolveTsFilename = ( const resolveRequest = ( request: string, parent: Module.Parent | undefined, - resolve: SimpleResolve, + nextResolve: SimpleResolve, ) => { // Support file protocol if (request.startsWith(fileUrlPrefix)) { @@ -102,25 +110,27 @@ const resolveRequest = ( const possiblePaths = tsconfigPathsMatcher(request); for (const possiblePath of possiblePaths) { - const tsFilename = resolveTsFilename(resolve, possiblePath, parent); + const tsFilename = resolveTsFilename(nextResolve, possiblePath, parent); if (tsFilename) { return tsFilename; } try { - return resolve(possiblePath); + return nextResolve(possiblePath); } catch {} } } - // If extension exists - const resolvedTsFilename = resolveTsFilename(resolve, request, parent); - if (resolvedTsFilename) { - return resolvedTsFilename; + // It should only try to resolve TS extensions first if it's a local file (non dependency) + if (isFilePath(request)) { + const resolvedTsFilename = resolveTsFilename(nextResolve, request, parent); + if (resolvedTsFilename) { + return resolvedTsFilename; + } } try { - return resolve(request); + return nextResolve(request); } catch (error) { const nodeError = error as NodeError; @@ -133,7 +143,7 @@ const resolveRequest = ( const isExportsPath = nodeError.message.match(/^Cannot find module '([^']+)'$/); if (isExportsPath) { const exportsPath = isExportsPath[1]; - const tsFilename = resolveTsFilename(resolve, exportsPath, parent); + const tsFilename = resolveTsFilename(nextResolve, exportsPath, parent); if (tsFilename) { return tsFilename; } @@ -142,7 +152,7 @@ const resolveRequest = ( const isMainPath = nodeError.message.match(/^Cannot find module '([^']+)'. Please verify that the package.json has a valid "main" entry$/); if (isMainPath) { const mainPath = isMainPath[1]; - const tsFilename = resolveTsFilename(resolve, mainPath, parent); + const tsFilename = resolveTsFilename(nextResolve, mainPath, parent); if (tsFilename) { return tsFilename; } @@ -222,17 +232,16 @@ export const createResolveFilename = ( return nextResolve(request, parent, ...restOfArgs); } - let _nextResolve = nextResolve; - if (namespace) { - /** - * When namespaced, the loaders are registered to the extensions in a hidden way - * so Node's built-in resolver will not try those extensions - * - * To support implicit extensions, we need to enhance the resolver with our own - * re-implementation of the implicit extension resolution - */ - _nextResolve = createImplicitResolver(_nextResolve); - } + /** + * Custom implicit resolver to resolve .ts over .js extensions + * + * To support implicit extensions, we need to enhance the resolver with our own + * re-implementation of the implicit extension resolution + * + * Also, when namespaced, the loaders are registered to the extensions in a hidden way + * so Node's built-in resolver will not try those extensions + */ + const _nextResolve = createImplicitResolver(nextResolve); const resolve: SimpleResolve = request_ => _nextResolve( request_, diff --git a/src/cjs/api/resolve-implicit-extensions.ts b/src/cjs/api/resolve-implicit-extensions.ts index 7943ba5f9..b3df6c8cc 100644 --- a/src/cjs/api/resolve-implicit-extensions.ts +++ b/src/cjs/api/resolve-implicit-extensions.ts @@ -1,21 +1,17 @@ import path from 'node:path'; import type { NodeError } from '../../types.js'; -import { isBarePackageNamePattern } from '../../utils/path-utils.js'; +import { isDirectoryPattern } from '../../utils/path-utils.js'; +import { mapTsExtensions } from '../../utils/map-ts-extensions.js'; import type { ResolveFilename } from './types.js'; -export const implicitlyResolvableExtensions = [ - '.ts', - '.tsx', - '.jsx', -] as const; - const tryExtensions = ( resolve: ResolveFilename, ...args: Parameters ) => { - for (const extension of implicitlyResolvableExtensions) { + const tryPaths = mapTsExtensions(args[0]); + for (const tryPath of tryPaths) { const newArgs = args.slice() as Parameters; - newArgs[0] += extension; + newArgs[0] = tryPath; try { return resolve(...newArgs); @@ -29,13 +25,40 @@ export const createImplicitResolver = ( request, ...args ) => { + if (request === '.') { + request = './'; + } + + /** + * Currently, there's an edge case where it doesn't resolve index.ts over index.js + * if the request doesn't end with a slash. e.g. `import './dir'` + * Doesn't handle '.' either + */ + if (isDirectoryPattern.test(request)) { + // If directory, can be index.js, index.ts, etc. + let joinedPath = path.join(request, 'index'); + + /** + * path.join will remove the './' prefix if it exists + * but it should only be added back if it was there before + * (e.g. not package directory imports) + */ + if (request.startsWith('./')) { + joinedPath = `./${joinedPath}`; + } + + const resolved = tryExtensions(resolve, joinedPath, ...args); + if (resolved) { + return resolved; + } + } + try { return resolve(request, ...args); } catch (_error) { const nodeError = _error as NodeError; if ( nodeError.code === 'MODULE_NOT_FOUND' - && !isBarePackageNamePattern.test(request) ) { const resolved = ( tryExtensions(resolve, request, ...args) diff --git a/src/esm/hook/load.ts b/src/esm/hook/load.ts index 0df3a6044..e4965623d 100644 --- a/src/esm/hook/load.ts +++ b/src/esm/hook/load.ts @@ -1,4 +1,5 @@ import { fileURLToPath } from 'node:url'; +import path from 'node:path'; import type { LoadHook } from 'node:module'; import { readFile } from 'node:fs/promises'; import type { TransformOptions } from 'esbuild'; @@ -121,7 +122,11 @@ export const load: LoadHook = async ( code, filePath, { - tsconfigRaw: fileMatcher?.(filePath) as TransformOptions['tsconfigRaw'], + tsconfigRaw: ( + path.isAbsolute(filePath) + ? fileMatcher?.(filePath) as TransformOptions['tsconfigRaw'] + : undefined + ), }, ); diff --git a/src/esm/hook/resolve.ts b/src/esm/hook/resolve.ts index 2470ce3ff..7ea351dc2 100644 --- a/src/esm/hook/resolve.ts +++ b/src/esm/hook/resolve.ts @@ -14,7 +14,7 @@ import { fileUrlPrefix, tsExtensionsPattern, isDirectoryPattern, - isBarePackageNamePattern, + isRelativePath, } from '../../utils/path-utils.js'; import type { TsxRequest } from '../types.js'; import { @@ -61,7 +61,7 @@ const resolveExtensions = async ( nextResolve: NextResolve, throwError?: boolean, ) => { - const tryPaths = mapTsExtensions(url, true); + const tryPaths = mapTsExtensions(url); if (!tryPaths) { return; } @@ -93,16 +93,18 @@ const resolveBase: ResolveHook = async ( context, nextResolve, ) => { - const isBarePackageName = isBarePackageNamePattern.test(specifier); - - // Typescript gives .ts, .cts, or .mts priority over actual .js, .cjs, or .mjs extensions - // - // If `allowJs` is set in `tsconfig.json`, then we'll apply the same resolution logic - // to files without a TypeScript extension. + /** + * Only prioritize TypeScript extensions for file paths (no dependencies) + * TS aliases are pre-resolved so they're file paths + * + * If `allowJs` is set in `tsconfig.json`, then we'll apply the same resolution logic + * to files without a TypeScript extension. + */ if ( - // Ignore if there's no subpath to test extensions against - !isBarePackageName - && ( + ( + specifier.startsWith(fileUrlPrefix) + || isRelativePath(specifier) + ) && ( tsExtensionsPattern.test(context.parentURL!) || allowJs ) @@ -139,10 +141,18 @@ const resolveDirectory: ResolveHook = async ( context, nextResolve, ) => { + if (specifier === '.') { + specifier = './'; + } + if (isDirectoryPattern.test(specifier)) { + const urlParsed = new URL(specifier, context.parentURL); + // If directory, can be index.js, index.ts, etc. + urlParsed.pathname = path.join(urlParsed.pathname, 'index'); + return (await resolveExtensions( - `${specifier}index`, + urlParsed.toString(), context, nextResolve, true, diff --git a/src/utils/map-ts-extensions.ts b/src/utils/map-ts-extensions.ts index e074227c5..34f3a68c6 100644 --- a/src/utils/map-ts-extensions.ts +++ b/src/utils/map-ts-extensions.ts @@ -1,7 +1,21 @@ import path from 'node:path'; +import { isFilePath, fileUrlPrefix, nodeModulesPath } from './path-utils.js'; -const noExtension = ['.js', '.json', '.ts', '.tsx', '.jsx']; +const implicitJsExtensions = ['.js', '.json']; +const implicitTsExtensions = ['.ts', '.tsx', '.jsx']; +// Guess extension +const localExtensions = [...implicitTsExtensions, ...implicitJsExtensions]; + +/** + * If dependency, prioritize .js extensions over .ts + * + * .js is more likely to behave correctly than the .ts file + * https://github.com/evanw/esbuild/releases/tag/v0.20.0 + */ +const dependencyExtensions = [...implicitJsExtensions, ...implicitTsExtensions]; + +// Swap extension const tsExtensions: Record = Object.create(null); tsExtensions['.js'] = ['.ts', '.tsx', '.js', '.jsx']; tsExtensions['.jsx'] = ['.tsx', '.ts', '.jsx', '.js']; @@ -10,28 +24,47 @@ tsExtensions['.mjs'] = ['.mts']; export const mapTsExtensions = ( filePath: string, - handleMissingExtension?: boolean, ) => { const splitPath = filePath.split('?'); - let [pathname] = splitPath; + const pathQuery = splitPath[1] ? `?${splitPath[1]}` : ''; + const [pathname] = splitPath; const extension = path.extname(pathname); - let tryExtensions = tsExtensions[extension]; + const tryPaths: string[] = []; + + const tryExtensions = tsExtensions[extension]; if (tryExtensions) { - pathname = pathname.slice(0, -extension.length); - } else { - if (!handleMissingExtension) { - return; - } + const extensionlessPath = pathname.slice(0, -extension.length); - tryExtensions = noExtension; + tryPaths.push( + ...tryExtensions.map( + extension_ => ( + extensionlessPath + + extension_ + + pathQuery + ), + ), + ); } - return tryExtensions.map( - tsExtension => ( - pathname - + tsExtension - + (splitPath[1] ? `?${splitPath[1]}` : '') + const guessExtensions = ( + ( + !(filePath.startsWith(fileUrlPrefix) || isFilePath(pathname)) + || pathname.includes(nodeModulesPath) + || pathname.includes('/node_modules/') // For file:// URLs on Windows + ) + ? dependencyExtensions + : localExtensions + ); + tryPaths.push( + ...guessExtensions.map( + extension_ => ( + pathname + + extension_ + + pathQuery + ), ), ); + + return tryPaths; }; diff --git a/src/utils/path-utils.ts b/src/utils/path-utils.ts index 373c8dfd1..40d286431 100644 --- a/src/utils/path-utils.ts +++ b/src/utils/path-utils.ts @@ -20,7 +20,7 @@ export const isRelativePath = (request: string) => ( ) ); -const isUnixPath = (request: string) => ( +export const isFilePath = (request: string) => ( isRelativePath(request) || path.isAbsolute(request) ); @@ -29,7 +29,7 @@ const isUnixPath = (request: string) => ( export const requestAcceptsQuery = (request: string) => { // ./foo.js?query // /foo.js?query in UNIX - if (isUnixPath(request)) { + if (isFilePath(request)) { return true; } @@ -57,3 +57,5 @@ export const isDirectoryPattern = /\/(?:$|\?)/; // Only matches packages names without subpaths (e.g. `foo` but not `foo/bar`) // Back slash included to exclude Windows paths export const isBarePackageNamePattern = /^(?:@[^/]+\/)?[^/\\]+$/; + +export const nodeModulesPath = `${path.sep}node_modules${path.sep}`; diff --git a/tests/fixtures.ts b/tests/fixtures.ts index ac676ca64..29e2ee2a3 100644 --- a/tests/fixtures.ts +++ b/tests/fixtures.ts @@ -212,6 +212,8 @@ export const files = { `, 'period.in.name.ts': 'export { a } from "."', + + 'index.js': 'throw new Error("should not be loaded")', }, // TODO: test resolution priority for files 'index.tsx` & 'index.tsx.ts` via 'index.tsx' @@ -310,10 +312,16 @@ export const files = { }, 'pkg-exports': { 'package.json': createPackageJson({ - type: 'module', - exports: './index.js', + exports: { + '.': './index.js', + './file': './file.js', + './file.js': './error.js', + './file.ts': './error.js', + }, }), 'index.ts': syntaxLowering, + 'file.js': syntaxLowering, + 'error.js': 'throw new Error("should not be loaded")', }, }, }; diff --git a/tests/specs/smoke.ts b/tests/specs/smoke.ts index ae049a029..9a5a8e358 100644 --- a/tests/specs/smoke.ts +++ b/tests/specs/smoke.ts @@ -41,6 +41,8 @@ export default testSuite(async ({ describe }, { tsx, supports, version }: NodeAp import 'pkg-module/index'; import 'pkg-module/empty-export'; // implicit directory & extension + import 'pkg-exports/file'; + // .js in esm syntax import * as js from './js/index.js'; import './js/index.js?query=123'; @@ -197,6 +199,8 @@ export default testSuite(async ({ describe }, { tsx, supports, version }: NodeAp import * as pkgModule from 'pkg-module'; import 'pkg-module/index'; + import 'pkg-exports/file'; + // Resolving TS files in dependencies (e.g. implicit extensions & export maps) import 'pkg-commonjs/ts.js'; import 'pkg-module/ts.js';