Skip to content

Commit

Permalink
Revert "fix(test runner): align with typescript behaviour for resolvi…
Browse files Browse the repository at this point in the history 
…ng `index.js` and `package.json` through path mapping (#32078)" (#32492)

This reverts commit effb1ae.

This broke path mapping into directories in ESM mode. References #32480.
  • Loading branch information
@dgozman
dgozman committed Sep 6, 2024
1 parent 3fe1263 commit 1402dee
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 167 deletions.
2 changes: 1 addition & 1 deletion packages/playwright-ct-core/src/vitePlugin.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>();
Expand Down
6 changes: 3 additions & 3 deletions packages/playwright-ct-core/src/viteUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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'});`);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/playwright/src/transform/esmLoader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
18 changes: 6 additions & 12 deletions packages/playwright/src/transform/transform.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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;
}
Expand Down
19 changes: 4 additions & 15 deletions packages/playwright/src/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
}
}

Expand Down
135 changes: 0 additions & 135 deletions tests/playwright-test/resolver.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' });

Expand Down Expand Up @@ -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'`);
});

0 comments on commit 1402dee

Please sign in to comment.