fix(test runner): align with typescript behaviour for resolving index.js and package.json through path mapping
#32078
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…x.js` and `package.json` through path mapping Co-Authored-By: Dmitry Gozman <dgozman@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dgozman
reviewed
Aug 9, 2024
| // TypeScript does not interpret package.json for path mappings: https://www.typescriptlang.org/docs/handbook/modules/reference.html#paths-should-not-point-to-monorepo-packages-or-node_modules-packages | ||
| const shouldNotResolveDirectory = isPathMapping && isESM; | ||
|
|
||
| if (!shouldNotResolveDirectory && dirExists(resolved)) { |
There was a problem hiding this comment.
It seems like TypeScript:
- does not honor
exportsfield frompackage.jsonafter the type mapping; - does honor the default
index.jsandmainfield frompackage.jsonafter the type mapping as explained here: https://www.typescriptlang.org/docs/handbook/modules/reference.html#directory-modules-index-file-resolution. Note that bothindex.jsandmainare ignored for ESM, as explained here: https://nodejs.org/dist/latest-v20.x/docs/api/modules.html#folders-as-modules.
The docs above match my manual testing as well. I think we should follow TS and implement the same logic.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- For the
exportsfield, it looks like the code will defer to Node.js (when not in ESM), and Node will honor theexports. Is that not the case? - For the
mainfield, 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!
There was a problem hiding this comment.
- 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.
- The
.tsextension 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.
There was a problem hiding this comment.
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!
This comment has been minimized.
This comment has been minimized.
dgozman
approved these changes
Aug 12, 2024
| // TypeScript does not interpret package.json for path mappings: https://www.typescriptlang.org/docs/handbook/modules/reference.html#paths-should-not-point-to-monorepo-packages-or-node_modules-packages | ||
| const shouldNotResolveDirectory = isPathMapping && isESM; | ||
|
|
||
| if (!shouldNotResolveDirectory && dirExists(resolved)) { |
There was a problem hiding this comment.
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!
Test results for "tests 1"3 flaky29693 passed, 711 skipped Merge workflow run. |
dgozman
added a commit
to dgozman/playwright
that referenced
this pull request
Sep 6, 2024
This regressed in microsoft#32078.
dgozman
added a commit
to dgozman/playwright
that referenced
this pull request
Sep 6, 2024
…ng `index.js` and `package.json` through path mapping (microsoft#32078)" This reverts commit effb1ae.
dgozman
added a commit
that referenced
this pull request
Sep 6, 2024
dgozman
added a commit
to dgozman/playwright
that referenced
this pull request
Sep 6, 2024
This regressed in microsoft#32078.
dgozman
added a commit
to dgozman/playwright
that referenced
this pull request
Sep 9, 2024
This regressed in microsoft#32078.
dgozman
added a commit
to dgozman/playwright
that referenced
this pull request
Sep 9, 2024
This regressed in microsoft#32078.
dgozman
added a commit
to dgozman/playwright
that referenced
this pull request
Sep 11, 2024
…pescript behaviour for resolving `index.js` and `package.json` through path mapping (microsoft#32078)" This reverts commit effb1ae. This broke path mapping into directories in ESM mode. References microsoft#32480.
dgozman
added a commit
to dgozman/playwright
that referenced
this pull request
Sep 11, 2024
…ts with path mapping We now hopefully align with `moduleResolution: bundler` tsconfig option, allowing directory imports in every scenario, and allowing proper module imports when not going through the type mapping. This regressed in microsoft#32078. Fixes microsoft#32480, fixes microsoft#31811.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theindex.jsfile in that directory if it's in CommonJS mode.We need to mirror this in our
esmLoader.ts.