-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(typescript): allow directory imports #23254
Conversation
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! Just a few comments.
packages/playwright-test/src/util.ts
Outdated
|
||
function dirExists(resolved: string) { | ||
return fs.existsSync(resolved) && fs.lstatSync(resolved).isDirectory(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a newline at the end of the file.
@dgozman Good suggestions, thanks. I've updated |
not totally sure what happened to these checks, maybe we should re run them? |
@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 - |
I've added a try/catch and replaced |
@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 |
This looks very good. Let's avoid the
Why don't we pass
This seems unrelated to your change, we'll take a look on our own. Sorry for the inconvenience. |
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! |
Made a PR here: #23429 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kristojorg The code looks great, but linter is unhappy. With #23429 landed, this probably just needs a rebase?
packages/playwright-test/src/util.ts
Outdated
} | ||
|
||
function fileExists(resolved: string) { | ||
return fs.lstatSync(resolved)?.isFile(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Why
lstat
and notstat
? 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 fordirExists
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Co-authored-by: Dmitry Gozman <dgozman@gmail.com> Signed-off-by: Kristo Jorgenson <kristojorg@users.noreply.github.com>
996d944
to
592d7b9
Compare
"tests 1" report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, merging in. Thank you for the PR!
"tests 1" report. |
Hey there, I'm wondering if you know when this will be released? |
This should be already included in v1.35.0. |
I see, yes it is in that release. It seems this isn't working for directory imports from
|
Can you file an issue and we can continue there? Easier to track, thanks! |
This updates previous work in #22887 to align more fully with
--moduleResolution=bundler
, allowing index files to be imported with the /index extension