fix(typescript): allow directory imports by kristojorg · Pull Request #23254 · microsoft/playwright

kristojorg · 2023-05-24T08:48:01Z

This updates previous work in #22887 to align more fully with --moduleResolution=bundler, allowing index files to be imported with the /index extension

kristojorg · 2023-05-24T08:49:22Z

@microsoft-github-policy-service agree

dgozman

Thank you for the PR! Just a few comments.

packages/playwright-test/src/util.ts

dgozman · 2023-05-24T18:58:08Z

packages/playwright-test/src/util.ts

+
+function dirExists(resolved: string) {
+  return fs.existsSync(resolved) && fs.lstatSync(resolved).isDirectory();
+}


Please add a newline at the end of the file.

packages/playwright-test/src/util.ts

tests/playwright-test/esm.spec.ts

kristojorg · 2023-05-25T07:20:32Z

@dgozman Good suggestions, thanks. I've updated

kristojorg · 2023-05-25T13:22:54Z

not totally sure what happened to these checks, maybe we should re run them?

kristojorg · 2023-05-30T09:24:52Z

@dgozman, wondering if there is something I can do to unblock this? The test failures look unrelated to the work I did

dgozman · 2023-05-30T15:52:10Z

@dgozman, wondering if there is something I can do to unblock this? The test failures look unrelated to the work I did

Sorry for delay, holidays here. The failures are indeed related - fs.lstatSync() throws, see my comments.

kristojorg · 2023-05-31T07:56:25Z

The failures are indeed related - fs.lstatSync() throws, see my comments.

I've added a try/catch and replaced lstatSync with statSync.

kristojorg · 2023-05-31T12:41:18Z

@dgozman I'm just struggling to understand the error the failing test is pointing at. It appears to me that the expect block is being incorrectly interpreted as an array of strings when in fact it is an array of regexs.

I also can't get it to fail locally

dgozman · 2023-05-31T16:15:07Z

This looks very good. Let's avoid the try/catch (see below), and should be good to go.

I've added a try/catch

Why don't we pass throwIfNoEntry: false option to avoid exceptions altogether? This should be much faster.

I'm just struggling to understand the error the failing test is pointing at.

This seems unrelated to your change, we'll take a look on our own. Sorry for the inconvenience.

kristojorg · 2023-06-01T09:40:48Z

Why don't we pass throwIfNoEntry: false option to avoid exceptions altogether? This should be much faster.

I tried this actually and Typescript was giving me an error that it was an invalid option. I figured it must have been added in a newer version of Node than we had installed, but it appears to just be missing from the 14.x types. I've added it with a comment and ts-ignore. Lmk if you'd like me to change that

mxschmitt · 2023-06-01T09:57:03Z

Why don't we pass throwIfNoEntry: false option to avoid exceptions altogether? This should be much faster.

I tried this actually and Typescript was giving me an error that it was an invalid option. I figured it must have been added in a newer version of Node than we had installed, but it appears to just be missing from the 14.x types. I've added it with a comment and ts-ignore. Lmk if you'd like me to change that

ts-ignore is unfortunately not an option for us. Let's upgrade @types/node to 16.x since we dropped support for Node.js 14? (as a separate PR). If you don't want to do it, I can do it later. Thanks!

kristojorg · 2023-06-01T10:07:58Z

Made a PR here: #23429

dgozman

@kristojorg The code looks great, but linter is unhappy. With #23429 landed, this probably just needs a rebase?

dgozman · 2023-05-30T15:46:10Z

packages/playwright-test/src/util.ts

+}
+
+function fileExists(resolved: string) {
+  return fs.lstatSync(resolved)?.isFile();


Why lstat and not stat? It will return "false" for a symbolic link, so we'd better check the resolved file instead.

We should pass throwIfNoEntry: false option to avoid exceptions. Same for dirExists.

Should be fixed!

This updates previous work in microsoft#22887 to align more fully with `--moduleResolution=bundler`, allowing index files to be imported with the /index extension

Co-authored-by: Dmitry Gozman <dgozman@gmail.com> Signed-off-by: Kristo Jorgenson <kristojorg@users.noreply.github.com>

github-actions · 2023-06-05T08:33:42Z

"tests 1" report.

dgozman

Looks great, merging in. Thank you for the PR!

github-actions · 2023-06-05T16:36:52Z

"tests 1" report.

kristojorg · 2023-06-27T15:29:15Z

Hey there, I'm wondering if you know when this will be released?

mxschmitt · 2023-06-27T15:33:47Z

This should be already included in v1.35.0.

kristojorg · 2023-06-28T10:13:33Z

I see, yes it is in that release. It seems this isn't working for directory imports from node_modules. For example if I have import pkg from "@org/package", I get a ENOENT: no such file or directory error. The path it is trying to stat however, is /my/source/code/@org/package. I have two questions:

Is there a different way to handle node_module imports?
Should I make this function throw a better error if neither a file or directory exists at the given location, even with the various extensions and index we try adding?

mxschmitt · 2023-06-28T10:37:49Z

Can you file an issue and we can continue there? Easier to track, thanks!

Regressed in microsoft#23254.

…26692) Regressed in #23254. Fixes #26650.

…icrosoft#26692) Regressed in microsoft#23254. Fixes microsoft#26650.

kristojorg mentioned this pull request May 24, 2023

feat(typescript): align with --moduleResolution=bundler #22887

Merged

dgozman reviewed May 24, 2023

View reviewed changes

kristojorg requested a review from dgozman May 25, 2023 07:19

kristojorg mentioned this pull request Jun 1, 2023

chore: upgrade @types/node to 16.x #23429

Merged

dgozman reviewed Jun 2, 2023

View reviewed changes

kristojorg and others added 9 commits June 5, 2023 09:45

fix(typescript): allow directory imports

0312808

This updates previous work in microsoft#22887 to align more fully with `--moduleResolution=bundler`, allowing index files to be imported with the /index extension

lint fix

a41249d

replace existsSync with lstatSync

a2da32d

Co-authored-by: Dmitry Gozman <dgozman@gmail.com> Signed-off-by: Kristo Jorgenson <kristojorg@users.noreply.github.com>

Apply suggestions from code review

e4b2c56

Co-authored-by: Dmitry Gozman <dgozman@gmail.com> Signed-off-by: Kristo Jorgenson <kristojorg@users.noreply.github.com>

add newline

9042197

replace lstatSync with statSync

9c1ebf7

add try/catch when statSync'ing

1ed06b4

use throwIfNoEntry: false

df3f034

remove ts-ignore

592d7b9

kristojorg force-pushed the resolve-dir-imports branch from 996d944 to 592d7b9 Compare June 5, 2023 07:50

dgozman approved these changes Jun 5, 2023

View reviewed changes

dgozman merged commit d5d155d into microsoft:main Jun 5, 2023

kristojorg deleted the resolve-dir-imports branch June 6, 2023 12:39

dgozman added a commit to dgozman/playwright that referenced this pull request Aug 24, 2023

fix(resolver): allow importing packages with non-index main script

d8c7bc4

Regressed in microsoft#23254.

dgozman mentioned this pull request Aug 24, 2023

fix(resolver): allow importing packages with non-index main script #26692

Merged

dgozman added a commit that referenced this pull request Aug 24, 2023

fix(resolver): allow importing packages with non-index main script (#…

c970179

…26692) Regressed in #23254. Fixes #26650.

Germandrummer92 pushed a commit to OctoMind-dev/playwright that referenced this pull request Oct 27, 2023

fix(resolver): allow importing packages with non-index main script (m…

54865d6

…icrosoft#26692) Regressed in microsoft#23254. Fixes microsoft#26650.

Skn0tt mentioned this pull request Jul 31, 2024

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

Closed

Conversation

kristojorg commented May 24, 2023

Uh oh!

kristojorg commented May 24, 2023

Uh oh!

dgozman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dgozman May 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kristojorg commented May 25, 2023

Uh oh!

kristojorg commented May 25, 2023

Uh oh!

kristojorg commented May 30, 2023

Uh oh!

dgozman commented May 30, 2023

Uh oh!

kristojorg commented May 31, 2023

Uh oh!

kristojorg commented May 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dgozman commented May 31, 2023

Uh oh!

kristojorg commented Jun 1, 2023

Uh oh!

mxschmitt commented Jun 1, 2023

Uh oh!

kristojorg commented Jun 1, 2023

Uh oh!

dgozman left a comment

Choose a reason for hiding this comment

Uh oh!

dgozman May 30, 2023

Choose a reason for hiding this comment

Uh oh!

kristojorg Jun 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 5, 2023

Uh oh!

dgozman left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 5, 2023

Uh oh!

kristojorg commented Jun 27, 2023

Uh oh!

mxschmitt commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristojorg commented Jun 28, 2023

Uh oh!

mxschmitt commented Jun 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kristojorg commented May 31, 2023 •

edited

Loading

kristojorg Jun 5, 2023 •

edited

Loading

mxschmitt commented Jun 27, 2023 •

edited

Loading