fix(test runner): align with typescript behaviour for resolving `index.js` and `package.json` through path mapping by Skn0tt · Pull Request #32078 · microsoft/playwright

Skn0tt · 2024-08-08T11:55:59Z

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.

…x.js` and `package.json` through path mapping Co-Authored-By: Dmitry Gozman <dgozman@gmail.com>

dgozman · 2024-08-09T12:33:37Z

packages/playwright/src/util.ts

+  // 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)) {


It seems like TypeScript:

does not honor exports field from package.json after the type mapping;

does honor the default index.js and main field from package.json after the type mapping as explained here: https://www.typescriptlang.org/docs/handbook/modules/reference.html#directory-modules-index-file-resolution. Note that both index.js and main are 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.

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.

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!

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 .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.

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!

dgozman · 2024-08-12T12:53:36Z

packages/playwright/src/util.ts

+  // 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)) {


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!

github-actions · 2024-08-12T13:12:09Z

Test results for "tests 1"

3 flaky

⚠️ [playwright-test] › ui-mode-test-progress.spec.ts:22:5 › should update trace live
⚠️ [webkit-library] › library/video.spec.ts:165:5 › screencast › should work with old options
⚠️ [webkit-page] › page/page-goto.spec.ts:182:3 › should properly cancel Cross-Origin-Opener-Policy navigation

29693 passed, 711 skipped
✔️✔️✔️

Merge workflow run.

This regressed in microsoft#32078.

…ng `index.js` and `package.json` through path mapping (microsoft#32078)" This reverts commit effb1ae.

…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.

This regressed in microsoft#32078.

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 #32078. Fixes #32480, fixes #31811.

…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.

#32560) …behaviour for resolving `index.js` and `package.json` through path mapping (#32078)" This reverts commit effb1ae. This broke path mapping into directories in ESM mode. References #32480.

…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.

…ath mapping (#32571) 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 #32078. Fixes #32480, fixes #31811.

fix(test runner): align with typescript behaviour for resolving `inde…

84690a5

…x.js` and `package.json` through path mapping Co-Authored-By: Dmitry Gozman <dgozman@gmail.com>

Skn0tt requested a review from dgozman August 8, 2024 11:56

Skn0tt self-assigned this Aug 8, 2024

Skn0tt mentioned this pull request Aug 8, 2024

fix(runner): don't stumble over directory imports #31915

Closed

This comment has been minimized.

Sign in to view

fix test

ca1ad4d

This comment has been minimized.

Sign in to view

dgozman reviewed Aug 9, 2024

View reviewed changes

add test for exports field

9a80386

This comment has been minimized.

Sign in to view

add test, update docs

62ff3fd

dgozman approved these changes Aug 12, 2024

View reviewed changes

Skn0tt merged commit effb1ae into microsoft:main Aug 12, 2024

dgozman added a commit to dgozman/playwright that referenced this pull request Sep 6, 2024

fix(test runner): allow directory imports with path mapping

f2b9a33

This regressed in microsoft#32078.

dgozman mentioned this pull request Sep 6, 2024

fix(test runner): allow directory imports with path mapping #32491

Merged

dgozman added a commit to dgozman/playwright that referenced this pull request Sep 6, 2024

Revert "fix(test runner): align with typescript behaviour for resolvi…

2891162

…ng `index.js` and `package.json` through path mapping (microsoft#32078)" This reverts commit effb1ae.

dgozman added a commit to dgozman/playwright that referenced this pull request Sep 6, 2024

fix(test runner): allow directory imports with path mapping

f555d90

This regressed in microsoft#32078.

dgozman added a commit to dgozman/playwright that referenced this pull request Sep 9, 2024

fix(test runner): allow directory imports with path mapping

00b37ab

This regressed in microsoft#32078.

dgozman added a commit to dgozman/playwright that referenced this pull request Sep 9, 2024

fix(test runner): allow directory imports with path mapping

52589e8

This regressed in microsoft#32078.

dgozman mentioned this pull request Sep 11, 2024

cherry-pick(#32492): Revert "fix(test runner): align with typescript … #32560

Merged

dgozman mentioned this pull request Sep 11, 2024

cherry-pick(#32491): fix(test runner): allow directory imports with path mapping #32571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

fix(test runner): align with typescript behaviour for resolving `index.js` and `package.json` through path mapping#32078
Skn0tt merged 4 commits intomicrosoft:mainfrom
Skn0tt:fix-31811-2

Skn0tt commented Aug 8, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

dgozman Aug 9, 2024

Uh oh!

Skn0tt Aug 9, 2024

Uh oh!

dgozman Aug 9, 2024

Uh oh!

Skn0tt Aug 12, 2024

Uh oh!

dgozman Aug 12, 2024

Uh oh!

This comment has been minimized.

dgozman Aug 12, 2024

Uh oh!

github-actions bot commented Aug 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

Skn0tt commented Aug 8, 2024

Uh oh!

This comment has been minimized.

This comment has been minimized.

dgozman Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

Skn0tt Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

dgozman Aug 9, 2024

Choose a reason for hiding this comment

Uh oh!

Skn0tt Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

dgozman Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

dgozman Aug 12, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 12, 2024

Test results for "tests 1"

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants