Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(test runner): align with typescript behaviour for resolving index.js and package.json through path mapping #32078

Merged
merged 4 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/playwright-ct-core/src/vitePlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,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);
const importPath = resolveHook(importInfo.filename, importInfo.importSource, true);
if (!importPath)
continue;
const deps = new Set<string>();
Expand Down
6 changes: 3 additions & 3 deletions packages/playwright-ct-core/src/viteUtils.ts
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)).filter(Boolean) as string[]);
componentsByImportingFile.set(file, importList.map(i => resolveHook(i.filename, i.importSource, true)).filter(Boolean) as string[]);
}
}

export function hasJSComponents(components: ImportInfo[]): boolean {
for (const component of components) {
const importPath = resolveHook(component.filename, component.importSource);
const importPath = resolveHook(component.filename, component.importSource, true);
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) || value.importSource;
const importPath = resolveHook(value.filename, value.importSource, true) || 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
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);
const resolved = resolveHook(filename, specifier, true);
if (resolved !== undefined)
specifier = url.pathToFileURL(resolved).toString();
}
Expand Down
18 changes: 12 additions & 6 deletions packages/playwright/src/transform/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,21 @@ function loadAndValidateTsconfigsForFile(file: string): ParsedTsConfigData[] {
const pathSeparator = process.platform === 'win32' ? ';' : ':';
const builtins = new Set(Module.builtinModules);

export function resolveHook(filename: string, specifier: string): string | undefined {
export function resolveHook(filename: string, specifier: string, isESM: boolean): 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));

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).
*/
const isTypeScript = filename.endsWith('.ts') || filename.endsWith('.tsx');
const tsconfigs = loadAndValidateTsconfigsForFile(filename);
for (const tsconfig of tsconfigs) {
Expand Down Expand Up @@ -142,7 +148,7 @@ export function resolveHook(filename: string, specifier: string): string | undef
if (value.includes('*'))
candidate = candidate.replace('*', matchedPartOfSpecifier);
candidate = path.resolve(tsconfig.pathsBase!, candidate);
const existing = resolveImportSpecifierExtension(candidate);
const existing = resolveImportSpecifierExtension(candidate, true, isESM);
if (existing) {
longestPrefixLength = keyPrefix.length;
pathMatchedByLongestPrefix = existing;
Expand All @@ -156,7 +162,7 @@ export function resolveHook(filename: string, specifier: string): string | undef
if (path.isAbsolute(specifier)) {
// Handle absolute file paths like `import '/path/to/file'`
// Do not handle module imports like `import 'fs'`
return resolveImportSpecifierExtension(specifier);
return resolveImportSpecifierExtension(specifier, false, isESM);
}
}

Expand Down Expand Up @@ -238,7 +244,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);
const resolved = resolveHook(parent.filename, specifier, false);
if (resolved !== undefined)
specifier = resolved;
}
Expand Down
19 changes: 15 additions & 4 deletions packages/playwright/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ const kExtLookups = new Map([
['.mjs', ['.mts']],
['', ['.js', '.ts', '.jsx', '.tsx', '.cjs', '.mjs', '.cts', '.mts']],
]);
export function resolveImportSpecifierExtension(resolved: string): string | undefined {
export function resolveImportSpecifierExtension(resolved: string, isPathMapping: boolean, isESM: boolean): string | undefined {
if (fileExists(resolved))
return resolved;

Expand All @@ -319,14 +319,25 @@ export function resolveImportSpecifierExtension(resolved: string): string | unde
break; // Do not try '' when a more specific extension like '.jsx' matched.
}

if (dirExists(resolved)) {
// 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like TypeScript:

The docs above match my manual testing as well. I think we should follow TS and implement the same logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a test to ensure we follow your first observation in 9a80386.

For the second one, isn't that exactly the behaviour we have right now? If there's a package.json file, then we let Node.js deal with resolving to either index.js or main, depending on the ESM mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • For the exports field, it looks like the code will defer to Node.js (when not in ESM), and Node will honor the exports. Is that not the case?
  • For the main field, TypeScript also checks various extensions (e.g. ts) after following it, and we currently do not, because we leave it to Node.js. That's a minor thing, but it would be ideal to mirror TypeScript there as well.
  • Updating comments to explain these things would be great as well!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I believe the test https://github.com/Skn0tt/playwright/blob/9a80386c7d057027499609aa4d648549482474fa/tests/playwright-test/resolver.spec.ts#L609-L644 asserts just that behaviour. Maybe i'm looking at it wrong though.
  2. The .ts extension is covered by https://github.com/Skn0tt/playwright/blob/9a80386c7d057027499609aa4d648549482474fa/tests/playwright-test/resolver.spec.ts#L572-L607. What other extensions should we cover?

Both of these are about the path mapping case. Are you referring to the case where there's no path mapping?

In 62ff3fd, i've added a test to ensure that main is ignored in ESM mode, and updated the comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the tests look like they cover all the cases. I guess for the item 1, Node.js does not look at exports either, which works for us. Thank you for following through!

// 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);
return resolveImportSpecifierExtension(dirImport, isPathMapping, isESM);
}
}

Expand Down
135 changes: 135 additions & 0 deletions tests/playwright-test/resolver.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,79 @@ 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 @@ -693,3 +766,65 @@ 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'`);
});
Loading