Skip to content

Commit

Permalink
fix(test runner): allow directory imports with path mapping
Browse files Browse the repository at this point in the history
This regressed in microsoft#32078.
  • Loading branch information
dgozman committed Sep 6, 2024
1 parent 1402dee commit f555d90
Show file tree
Hide file tree
Showing 5 changed files with 476 additions and 49 deletions.
10 changes: 7 additions & 3 deletions packages/playwright/src/third_party/tsconfig-loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,13 @@ export interface LoadedTsConfig {
}

export function loadTsConfig(configPath: string): LoadedTsConfig[] {
const references: LoadedTsConfig[] = [];
const config = innerLoadTsConfig(configPath, references);
return [config, ...references];
try {
const references: LoadedTsConfig[] = [];
const config = innerLoadTsConfig(configPath, references);
return [config, ...references];
} catch (e) {
throw new Error(`Failed to load tsconfig file at ${configPath}:\n${e.message}`);
}
}

function resolveConfigFile(baseConfigFile: string, referencedConfigFile: string) {
Expand Down
11 changes: 8 additions & 3 deletions packages/playwright/src/transform/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,13 @@ export function resolveHook(filename: string, specifier: string): string | undef
return;

if (isRelativeSpecifier(specifier))
return resolveImportSpecifierExtension(path.resolve(path.dirname(filename), specifier));
return resolveImportSpecifierExtension(path.resolve(path.dirname(filename), specifier), false);

/**
* 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
* However, if path-mapping doesn't yield a result, TypeScript falls back to the default resolution through node_modules.
*/
const isTypeScript = filename.endsWith('.ts') || filename.endsWith('.tsx');
const tsconfigs = loadAndValidateTsconfigsForFile(filename);
for (const tsconfig of tsconfigs) {
Expand Down Expand Up @@ -179,7 +184,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);
if (existing) {
longestPrefixLength = keyPrefix.length;
pathMatchedByLongestPrefix = existing;
Expand All @@ -193,7 +198,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);
}
}

Expand Down
42 changes: 38 additions & 4 deletions packages/playwright/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,20 @@ function folderIsModule(folder: string): boolean {
return require(packageJsonPath).type === 'module';
}

const packageJsonMainFieldCache = new Map<string, string | undefined>();

function getMainFieldFromPackageJson(packageJsonPath: string) {
if (!packageJsonMainFieldCache.has(packageJsonPath)) {
let mainField: string | undefined;
try {
mainField = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8')).main;
} catch {
}
packageJsonMainFieldCache.set(packageJsonPath, mainField);
}
return packageJsonMainFieldCache.get(packageJsonPath);
}

// This follows the --moduleResolution=bundler strategy from tsc.
// https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#moduleresolution-bundler
const kExtLookups = new Map([
Expand All @@ -316,7 +330,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): string | undefined {
if (fileExists(resolved))
return resolved;

Expand All @@ -332,13 +346,33 @@ export function resolveImportSpecifierExtension(resolved: string): string | unde
}

if (dirExists(resolved)) {
const packageJsonPath = path.join(resolved, 'package.json');

if (isPathMapping) {
// After TypeScript path mapping, here's how directory imports are resolved
// with "moduleResolution:bundler" setting:
// - `package.json#exports` is not respected, because it's not a proper module
// - `package.json#main` is respected according to "folder as module" Node.js behavior
// - `index.js` default is respected according to "folder as module" Node.js behavior
//
// More info:
// - 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 mainField = getMainFieldFromPackageJson(packageJsonPath);
const mainFieldResolved = mainField ? resolveImportSpecifierExtension(path.resolve(resolved, mainField), isPathMapping) : undefined;
return mainFieldResolved || resolveImportSpecifierExtension(path.join(resolved, 'index'), isPathMapping);
}

// If we import a package, let Node.js figure out the correct import based on package.json.
if (fileExists(path.join(resolved, 'package.json')))
// This also covers the "main" field for "folder as module".
if (fileExists(packageJsonPath))
return resolved;

// Otherwise, try to find a corresponding index file.
// Fallback to "folder as module" Node.js behavior.
// https://nodejs.org/dist/latest-v20.x/docs/api/modules.html#folders-as-modules
const dirImport = path.join(resolved, 'index');
return resolveImportSpecifierExtension(dirImport);
return resolveImportSpecifierExtension(dirImport, isPathMapping);
}
}

Expand Down
4 changes: 2 additions & 2 deletions tests/playwright-test/playwright-test-fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,8 @@ export const test = base
const TSCONFIG = {
'compilerOptions': {
'target': 'ESNext',
'moduleResolution': 'node',
'module': 'commonjs',
'moduleResolution': 'bundler',
'module': 'esnext',
'strict': true,
'esModuleInterop': true,
'allowSyntheticDefaultImports': true,
Expand Down
Loading

0 comments on commit f555d90

Please sign in to comment.