fix(runner): don't stumble over directory imports#31915
fix(runner): don't stumble over directory imports#31915Skn0tt wants to merge 8 commits intomicrosoft:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
dgozman
left a comment
There was a problem hiding this comment.
Could you please put more details into the description? What do we currently resolve to? What should happen instead? Does this only manifest with path mappings in tsconfig.json?
| const filename = url.fileURLToPath(context.parentURL); | ||
| const resolved = resolveHook(filename, specifier); | ||
| if (resolved !== undefined) | ||
| if (resolved !== undefined && fs.statSync(resolved).isFile()) |
There was a problem hiding this comment.
Perhaps pass a flag to resolveHook instead to avoid resolving a directory?
There was a problem hiding this comment.
well, looks like that didn't work! #31915 (comment)
There was a problem hiding this comment.
Well, that means we don't understand something. Let's dig until we figure it out.
There was a problem hiding this comment.
Looking at the test, they use an import style that's not part of the ESM spec, but part of CJS resolution. It feels similar to --experimental-specifier-resolution=node, which never went stable. What's the history behind us supporting this?
There was a problem hiding this comment.
Oh, this is about moduleResolution: bundler, and TypeScript resolution is part of the riddle! Fun. #23254
There was a problem hiding this comment.
I'm not entirely sure why, but 9a873ab seems to have fixed tests.
| const filename = url.fileURLToPath(context.parentURL); | ||
| const resolved = resolveHook(filename, specifier); | ||
| if (resolved !== undefined) | ||
| if (resolved !== undefined && fs.statSync(resolved).isFile()) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I felt a little inconfident in this PR because it was one of my earliest changes to the codebase. I've reviewed the entire The important thing here is that while ESM does not allow directory imports, TypeScript does. This only shows with That's why the correct change is to only touch directory resolution when |
This comment has been minimized.
This comment has been minimized.
packages/playwright/src/util.ts
Outdated
| ['', ['.js', '.ts', '.jsx', '.tsx', '.cjs', '.mjs', '.cts', '.mts']], | ||
| ]); | ||
| export function resolveImportSpecifierExtension(resolved: string): string | undefined { | ||
| export function resolveImportSpecifierExtension(resolved: string, dontResolveDirectories: boolean): string | undefined { |
There was a problem hiding this comment.
I'd reverse the option for easier understanding: resolveDirectories: boolean.
packages/playwright/src/util.ts
Outdated
| } | ||
|
|
||
| if (dirExists(resolved)) { | ||
| if (dontResolveDirectories) |
There was a problem hiding this comment.
Let's make this check before doing an expensive dirExists() call.
| const builtins = new Set(Module.builtinModules); | ||
|
|
||
| export function resolveHook(filename: string, specifier: string): string | undefined { | ||
| export function resolveHook(filename: string, specifier: string, dontResolveDirectories = false): string | undefined { |
There was a problem hiding this comment.
Let's call the new argument isModule and make it required. This will expose all the places where we need to think about it.
There was a problem hiding this comment.
Could you explain why you'd call it isModule? Because of "module loader"?
The other callsites of resolveHook are inside component testing, and are used to turn Vite's import info into filenames. I think the changes we're making here also apply there, so I don't think we even need a parameter. I've removed it in 8fe370a.
Test results for "tests 1"3 fatal errors, not part of any test 3 flaky29501 passed, 710 skipped Merge workflow run. |
|
superceded by #32078 |
…x.js` and `package.json` through path mapping (#32078) Supercedes #31915, closes #31811. When TypeScript resolves a specifier via path mapping, it does not interpret `package.json`. If path mapping resolves to a directory, it only looks at the `index.js` file in that directory if it's in CommonJS mode. We need to mirror this in our `esmLoader.ts`. --------- Co-authored-by: Dmitry Gozman <dgozman@gmail.com>
Closes #31811
Right now, in a monorepo-like setup that involves
compilerOptions#pathsintsconfig.json, ouresmLoadermechanism can trip up. We're turning a directory path into a file URL, and Node.js doesn't like that. This happens around https://github.com/Skn0tt/playwright/blob/e1aac57dfa6bccabf7deff6c54c3684b753926eb/packages/playwright/src/util.ts#L322-L333, where we purposefully don't try resolving the index file based onpackage.json.