From 1402dee9e6589d0d01130809e9114048d23d2fc9 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 6 Sep 2024 12:08:10 -0700 Subject: [PATCH] Revert "fix(test runner): align with typescript behaviour for resolving `index.js` and `package.json` through path mapping (#32078)" (#32492) This reverts commit effb1ae2349aeef1c0bfb0b6cd886b9b4b32c8e5. This broke path mapping into directories in ESM mode. References #32480. --- packages/playwright-ct-core/src/vitePlugin.ts | 2 +- packages/playwright-ct-core/src/viteUtils.ts | 6 +- .../playwright/src/transform/esmLoader.ts | 2 +- .../playwright/src/transform/transform.ts | 18 +-- packages/playwright/src/util.ts | 19 +-- tests/playwright-test/resolver.spec.ts | 135 ------------------ 6 files changed, 15 insertions(+), 167 deletions(-) diff --git a/packages/playwright-ct-core/src/vitePlugin.ts b/packages/playwright-ct-core/src/vitePlugin.ts index 462730c61ca21..ec738c87009f0 100644 --- a/packages/playwright-ct-core/src/vitePlugin.ts +++ b/packages/playwright-ct-core/src/vitePlugin.ts @@ -262,7 +262,7 @@ function vitePlugin(registerSource: string, templateDir: string, buildInfo: Buil async writeBundle(this: PluginContext) { for (const importInfo of importInfos.values()) { - const importPath = resolveHook(importInfo.filename, importInfo.importSource, true); + const importPath = resolveHook(importInfo.filename, importInfo.importSource); if (!importPath) continue; const deps = new Set(); diff --git a/packages/playwright-ct-core/src/viteUtils.ts b/packages/playwright-ct-core/src/viteUtils.ts index d1160c16f0994..a9f7020999e90 100644 --- a/packages/playwright-ct-core/src/viteUtils.ts +++ b/packages/playwright-ct-core/src/viteUtils.ts @@ -147,13 +147,13 @@ export async function populateComponentsFromTests(componentRegistry: ComponentRe for (const importInfo of importList) componentRegistry.set(importInfo.id, importInfo); if (componentsByImportingFile) - componentsByImportingFile.set(file, importList.map(i => resolveHook(i.filename, i.importSource, true)).filter(Boolean) as string[]); + componentsByImportingFile.set(file, importList.map(i => resolveHook(i.filename, i.importSource)).filter(Boolean) as string[]); } } export function hasJSComponents(components: ImportInfo[]): boolean { for (const component of components) { - const importPath = resolveHook(component.filename, component.importSource, true); + const importPath = resolveHook(component.filename, component.importSource); const extname = importPath ? path.extname(importPath) : ''; if (extname === '.js' || (importPath && !extname && fs.existsSync(importPath + '.js'))) return true; @@ -183,7 +183,7 @@ export function transformIndexFile(id: string, content: string, templateDir: str lines.push(registerSource); for (const value of importInfos.values()) { - const importPath = resolveHook(value.filename, value.importSource, true) || value.importSource; + const importPath = resolveHook(value.filename, value.importSource) || value.importSource; lines.push(`const ${value.id} = () => import('${importPath?.replaceAll(path.sep, '/')}').then((mod) => mod.${value.remoteName || 'default'});`); } diff --git a/packages/playwright/src/transform/esmLoader.ts b/packages/playwright/src/transform/esmLoader.ts index 0ef0f29eaf083..c84d15146b8fb 100644 --- a/packages/playwright/src/transform/esmLoader.ts +++ b/packages/playwright/src/transform/esmLoader.ts @@ -26,7 +26,7 @@ import { fileIsModule } from '../util'; async function resolve(specifier: string, context: { parentURL?: string }, defaultResolve: Function) { if (context.parentURL && context.parentURL.startsWith('file://')) { const filename = url.fileURLToPath(context.parentURL); - const resolved = resolveHook(filename, specifier, true); + const resolved = resolveHook(filename, specifier); if (resolved !== undefined) specifier = url.pathToFileURL(resolved).toString(); } diff --git a/packages/playwright/src/transform/transform.ts b/packages/playwright/src/transform/transform.ts index 52d05dde90369..9ca80399fb7f5 100644 --- a/packages/playwright/src/transform/transform.ts +++ b/packages/playwright/src/transform/transform.ts @@ -129,21 +129,15 @@ function loadAndValidateTsconfigsForFolder(folder: string): ParsedTsConfigData[] const pathSeparator = process.platform === 'win32' ? ';' : ':'; const builtins = new Set(Module.builtinModules); -export function resolveHook(filename: string, specifier: string, isESM: boolean): string | undefined { +export function resolveHook(filename: string, specifier: string): string | undefined { if (specifier.startsWith('node:') || builtins.has(specifier)) return; if (!shouldTransform(filename)) return; if (isRelativeSpecifier(specifier)) - return resolveImportSpecifierExtension(path.resolve(path.dirname(filename), specifier), false, isESM); - - /** - * TypeScript discourages path-mapping into node_modules - * (https://www.typescriptlang.org/docs/handbook/modules/reference.html#paths-should-not-point-to-monorepo-packages-or-node_modules-packages). - * It seems like TypeScript tries path-mapping first, but does not look at the `package.json` or `index.js` files in ESM. - * If path-mapping doesn't yield a result, TypeScript falls back to the default resolution (typically node_modules). - */ + return resolveImportSpecifierExtension(path.resolve(path.dirname(filename), specifier)); + const isTypeScript = filename.endsWith('.ts') || filename.endsWith('.tsx'); const tsconfigs = loadAndValidateTsconfigsForFile(filename); for (const tsconfig of tsconfigs) { @@ -185,7 +179,7 @@ export function resolveHook(filename: string, specifier: string, isESM: boolean) if (value.includes('*')) candidate = candidate.replace('*', matchedPartOfSpecifier); candidate = path.resolve(tsconfig.pathsBase!, candidate); - const existing = resolveImportSpecifierExtension(candidate, true, isESM); + const existing = resolveImportSpecifierExtension(candidate); if (existing) { longestPrefixLength = keyPrefix.length; pathMatchedByLongestPrefix = existing; @@ -199,7 +193,7 @@ export function resolveHook(filename: string, specifier: string, isESM: boolean) if (path.isAbsolute(specifier)) { // Handle absolute file paths like `import '/path/to/file'` // Do not handle module imports like `import 'fs'` - return resolveImportSpecifierExtension(specifier, false, isESM); + return resolveImportSpecifierExtension(specifier); } } @@ -281,7 +275,7 @@ function installTransformIfNeeded() { const originalResolveFilename = (Module as any)._resolveFilename; function resolveFilename(this: any, specifier: string, parent: Module, ...rest: any[]) { if (parent) { - const resolved = resolveHook(parent.filename, specifier, false); + const resolved = resolveHook(parent.filename, specifier); if (resolved !== undefined) specifier = resolved; } diff --git a/packages/playwright/src/util.ts b/packages/playwright/src/util.ts index 4c75d9d84136e..0531f167a0442 100644 --- a/packages/playwright/src/util.ts +++ b/packages/playwright/src/util.ts @@ -316,7 +316,7 @@ const kExtLookups = new Map([ ['.mjs', ['.mts']], ['', ['.js', '.ts', '.jsx', '.tsx', '.cjs', '.mjs', '.cts', '.mts']], ]); -export function resolveImportSpecifierExtension(resolved: string, isPathMapping: boolean, isESM: boolean): string | undefined { +export function resolveImportSpecifierExtension(resolved: string): string | undefined { if (fileExists(resolved)) return resolved; @@ -331,25 +331,14 @@ export function resolveImportSpecifierExtension(resolved: string, isPathMapping: break; // Do not try '' when a more specific extension like '.jsx' matched. } - // After TypeScript path mapping, here's how directories with a `package.json` are resolved: - // - `package.json#exports` is not respected - // - `package.json#main` is respected only in CJS mode - // - `index.js` default is respected only in CJS mode - // - // More info: - // - https://www.typescriptlang.org/docs/handbook/modules/reference.html#paths-should-not-point-to-monorepo-packages-or-node_modules-packages - // - https://www.typescriptlang.org/docs/handbook/modules/reference.html#directory-modules-index-file-resolution - // - https://nodejs.org/dist/latest-v20.x/docs/api/modules.html#folders-as-modules - - const shouldNotResolveDirectory = isPathMapping && isESM; - - if (!shouldNotResolveDirectory && dirExists(resolved)) { + if (dirExists(resolved)) { // If we import a package, let Node.js figure out the correct import based on package.json. if (fileExists(path.join(resolved, 'package.json'))) return resolved; + // Otherwise, try to find a corresponding index file. const dirImport = path.join(resolved, 'index'); - return resolveImportSpecifierExtension(dirImport, isPathMapping, isESM); + return resolveImportSpecifierExtension(dirImport); } } diff --git a/tests/playwright-test/resolver.spec.ts b/tests/playwright-test/resolver.spec.ts index b87e22babbe45..5a0e91b099412 100644 --- a/tests/playwright-test/resolver.spec.ts +++ b/tests/playwright-test/resolver.spec.ts @@ -606,79 +606,6 @@ test('should import packages with non-index main script through path resolver', expect(result.output).toContain(`foo=42`); }); -test('should not honor `package.json#main` field in ESM mode', async ({ runInlineTest }) => { - const result = await runInlineTest({ - 'app/pkg/main.ts': ` - export const foo = 42; - `, - 'app/pkg/package.json': ` - { "main": "main.ts" } - `, - 'package.json': ` - { "name": "example-project", "type": "module" } - `, - 'playwright.config.ts': ` - export default {}; - `, - 'tsconfig.json': `{ - "compilerOptions": { - "baseUrl": ".", - "paths": { - "app/*": ["app/*"], - }, - }, - }`, - 'example.spec.ts': ` - import { foo } from 'app/pkg'; - import { test, expect } from '@playwright/test'; - test('test', ({}) => { - console.log('foo=' + foo); - }); - `, - }); - - expect(result.exitCode).toBe(1); - expect(result.output).toContain(`Cannot find package 'app'`); -}); - - -test('does not honor `exports` field after type mapping', async ({ runInlineTest }) => { - const result = await runInlineTest({ - 'app/pkg/main.ts': ` - export const filename = 'main.ts'; - `, - 'app/pkg/index.js': ` - export const filename = 'index.js'; - `, - 'app/pkg/package.json': JSON.stringify({ - exports: { '.': { require: './main.ts' } } - }), - 'package.json': JSON.stringify({ - name: 'example-project' - }), - 'playwright.config.ts': ` - export default {}; - `, - 'tsconfig.json': JSON.stringify({ - compilerOptions: { - baseUrl: '.', - paths: { - 'app/*': ['app/*'], - }, - } - }), - 'example.spec.ts': ` - import { filename } from 'app/pkg'; - import { test, expect } from '@playwright/test'; - test('test', ({}) => { - console.log('filename=' + filename); - }); - `, - }); - - expect(result.output).toContain('filename=index.js'); -}); - test('should respect tsconfig project references', async ({ runInlineTest }) => { test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/29256' }); @@ -766,65 +693,3 @@ test('should respect --tsconfig option', async ({ runInlineTest }) => { expect(result.exitCode).toBe(0); expect(result.output).not.toContain(`Could not`); }); - - -test('should resolve index.js in CJS after path mapping', async ({ runInlineTest }) => { - test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/31811' }); - - const result = await runInlineTest({ - '@acme/lib/index.js': ` - exports.greet = () => console.log('hello playwright'); - `, - '@acme/lib/index.d.ts': ` - export const greet: () => void; - `, - 'tests/hello.test.ts': ` - import { greet } from '@acme/lib'; - import { test } from '@playwright/test'; - test('hello', async ({}) => { - greet(); - }); - `, - 'tests/tsconfig.json': JSON.stringify({ - compilerOptions: { - 'paths': { - '@acme/*': ['../@acme/*'], - } - } - }) - }); - - expect(result.exitCode).toBe(0); - expect(result.passed).toBe(1); -}); - -test('should not resolve index.js in ESM after path mapping', async ({ runInlineTest }) => { - test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/31811' }); - - const result = await runInlineTest({ - '@acme/lib/index.js': ` - export const greet = () => console.log('hello playwright'); - `, - '@acme/lib/index.d.ts': ` - export const greet: () => void; - `, - 'tests/hello.test.ts': ` - import { greet } from '@acme/lib'; - import { test } from '@playwright/test'; - test('hello', async ({}) => { - greet(); - }); - `, - 'tests/tsconfig.json': JSON.stringify({ - compilerOptions: { - 'paths': { - '@acme/*': ['../@acme/*'], - } - } - }), - 'package.json': JSON.stringify({ type: 'module' }), - }); - - expect(result.exitCode).toBe(1); - expect(result.output).toContain(`Cannot find package '@acme/lib'`); -});