[BUG] tsconfig-loader tries to load non-existing tsconfig causing the correct config not to be used

[JonWallsten]

Context:

• Playwright Version: 1.23.1
• Operating System: Windows 10 x64
• Node.js version: 16.17.0
• Browser: All
• Extra: [any specific details about your environment]

Code Snippet

import { Environment } from '@oas/internal-lib-e2e';

Describe the bug

I'm unable to import my monorepo package using TypeScript paths in the runner's child process (it works in the main process).

After a debugging session with myself here: https://github.com/microsoft/playwright/discussions/17466, i have come to the conclusion that the tsconfig-loader (https://github.com/microsoft/playwright/blob/29ff00ead2e79ea9b5374513f98801745e93b80c/packages/playwright-test/src/third_party/tsconfig-loader.ts#L60) is trying to resolve a tsconfig that doesn't exists and then try to use that instead of the correct one in the package root (process.cwd() in the main process).

So here it is resolving a non-existent tsconfig file:

And then it's using that in the child process instead of the correct one with the paths:

Since the cwd for the child process is the testDir from the config file, and not the package root, no matter what I do, I will have to wrong path since I use ./tsconfig.json as TS_NODE_PROJECT. It will always be resolved to the wrong path. My guess is that you do not want to return a non-existent tsconfig.json or return it but then don't cache it in loadAndValidateTsconfigForFile here: https://github.com/microsoft/playwright/blob/42491ecc0867129b97fa2fbd4fe9c58ff0d3feff/packages/playwright-test/src/transform.ts#L79

[rwoll]

Thanks for the report and the debugging so far! There are a few layers here, so it will be best if you can provide a repro.

A repro will help ensure we are actually debugging the same issue you are facing and will minimize us guessing. This is especially helpful when there are several layers/pieces involved.

Ideally the repro is a GitHub repository we can clone, download, and run to see the issue ourselves. It should include the commands to run it, too.

Thanks!

[JonWallsten]

Hmm, creating a repro would take a lot of time since we have a pretty complex setup involving a monorepo and multiple packages. I can try to create a PR instead with a solution to the problem.

Edit: Okey, there's a lot of questions that needs to be answered. I'll start by illustration the the issue more clearly tomorrow when I'm back at work, with a file structure and my settings. If that's not enough I'll work on a repro. But I really think the illustration should be enough. I looked at the source code the first time today and think I understand what's going on, so you guys will definitely get it when I present it in a more structured way.

[rwoll]

@JonWallsten Thanks! Where possible, please paste the contents of files instead of screenshots. I'll review tomorrow again once you've had a chance to structure the suggestions/pointers. Having something I can copy and paste from to construct a repro/test case will help us resolve things faster!

Cheers!

[JonWallsten]

@rwoll: So I have a repro here. It was pretty easy to recreate. https://playwright-repro-tsconfig.stackblitz.io

Instructions:

1. Download source from Stackblitz
2. Run npm i in the root.
3. Go to packages/app
4. Run npm run playwright:debug
5. Open DevTools for Node
6. Set breakpoint in test.spec.ts on line 2.
7. Step into these functions:
8. Profit!

I would say the main issue here is that it's using cwd instead of process.cwd() when trying to resolve the tsconfig.json, unless you guys are deliberately looking for tsconfig.json files in the test suite?

I mean, I specify the TS_NODE_PROJECT relative to the process.cwd() in my package.json, so I don't see a point looking relative to the testDir (from Playwright config). BUT, if you guys really want to look for a tsconfig.json in the testDir you should at least not stop if it doesn't exists, but continue up in the file tree, or use process.cwd() instead. Then you should move up the tree stopping when you find one. I don't think it's a bad idea caching the result here:

But you should not cache undefined for a path just because the tsconfig.json file doesn't exist for that path:

So I guess one of these two solutions should be implemented: The static way: Always use process.cwd() when you use TS_NODE_PROJECT to to resolve the tsconfig.json so I have control of it. The dynamic way: Try resolving tsconfig.json with cwd first, if it does not exists, either move up the file tree until one is found, or fallback to process.cwd() depending on how dynamic you want to be.

The current solution of only using a tsconfig.json if it exists in the testDir is probably not a good solution.

I can create a PR if you just decide which way to go.

[pavelfeldman]

When resolving config for the TypeScript file, resolution is based on the filesystem nesting, so I'm not sure why cwd is used. We are inheriting this logic from tsloader, so we should have probably cleaned it up further.

[dgozman]

@JonWallsten Thank you for the PR! Before we proceed, I'd like to clarify a few things:

1. Looking at the repro, it seems like you are running playwright with ts-node:

cross-env TS_NODE_PROJECT=./tsconfig.json node -r ts-node/register -r tsconfig-paths/register --inspect-brk ../../node_modules/@playwright/test/cli test

I don't think this is supported. Playwright supports TypeScript out of the box, and so does its own processing. If you use ts-node, there are no guarantees anything would work.

Could you please try without ts-node, with built-in TypeScript support? If that somehow does not work for you, we recommend compiling manually as a last resort.

2. Do you use TS_NODE_PROJECT just to configure ts-node? I think it's a mistake this variable is respected by Playwright, and we should drop that.

Let me know what you think.

[JonWallsten]

@dgozman: Oh, I did not know that. I just assumed TS-node was needed. My bad! I'll try the same thing without it. I can tell though, that besides failing reading tsconfig.json causing paths not to work, everything works fine. But if you support TypeScript there's really no point in running ts-node, just adds another layer of complexity.

I use TS_NODE_PROJECT to point to a specific tsconfig (tsconfig.e2e.json) instead of the default one.

I'll try without ts-node and check if I get a different result.

[JonWallsten]

@dgozman: I have removed the ts-node part, but it didn't change the outcome. It still uses the wrong directory to resolve tsconfig.json. And the fix I did still solves it. The good thing though is that I don't need ts-node.

[JonWallsten]

Related issue: https://github.com/microsoft/playwright/issues/12829

[dgozman]

@JonWallsten You should stop passing TS_NODE_PROJECT environment variable. As I said, it unfortunately confuses our tsconfig loader, and we'll remove it from future releases. If you do that, tsconfig should be resolved to the closest config next to the source/test file. Let me know if that works for you.

[JonWallsten]

@dgozman But then it won't find my tsconfig.e2e.json since it will only look for tsconfig.json. I have a feeling you guys really don't want to support this, but I don't understand why? It's supported in the rest of the tools we use and it was supported in protractor, which we are replacing.

[dgozman]

@JonWallsten We would like to understand the scenario to make an informed decision. If you have a usecase that does not work, we would be happy to help you!

In the stackblitz repo above, I don't see the tsconfig.e2e.json file, so I am lost. Could you please explain what are you trying to achieve, what files do you have, what configs, and which config should apply?

For the reference, we usually have a root tsconfig.json that configures the source, and also put a separate tests/tsconfig.json file that has some test-specific options. This setup just works for us. That said, if this does not work for you, we would love to know why.

[JonWallsten]

@dgozman: Sorry, that was just a minimal reproduction for the bug and not the actual setup.

Yeah, I started thinking after I wrote my previous reply if that would work for us. I need to check tomorrow when I'm at work. But I guess it could work. It's just not how we're used to do it. We have all tsconfig files in the package root with different names. But I guess I can get used to separating them.

[JonWallsten]

@dgozman: I did some tests, and there seems to be an issue that you guys don't adress that TS-node does, that causes our tests to fail. It's related to, but not directly caused by this issue (I think you can safely remove the TS_NODE_PROJECT flag since you can use the hierarchy as you mentioned).

The issue is in: https://github.com/microsoft/playwright/blob/9f17ee68719c75abbe3fdfce2ffecf8d032d47a3/packages/playwright-test/src/transform.ts#L96

When using a monorepo, we uses paths to point to the different packages. But we're pointing to the dist folders. And the dist folders contains the compiled and bundled code in js-format. And since you guys are only loading the tsconfig file for TypeScript files it can't resolve our packages if they are linked in another package.

As you can see in the image below, it processes internal-lib-e2e and that in turn uses our web-lib-core. But it can't resolve web-lib-core since our paths are missing because it doesn't read the tsconfig since it's not a TypeScript file.

So is there any other way of providing these paths?

Edit: It works fine with ts-node: node -r ts-node/register -r tsconfig-paths/register ../../node_modules/@playwright/test/cli test Most likely since it uses tsconfig-paths/register

[dgozman]

@JonWallsten If I understood you correctly, you want paths from tsconfig.json to apply when require'ing from a compiled .js file? This is not something Playwright supports, because it assumes that compiled code is ready to run "as is". I see that ts-node transforms javascript files when allowJs is set in the config, but I am not sure we should go this way. Perhaps you could point your paths to the source directory instead of the dist directory?

[JonWallsten]

@dgozman: Yeah, exactly. I point my paths to the dist-folder in each package so it mimics the behavior of being a regular installed dependency. But since they are not located in node_modules they can't be resolved without paths. This works great for one level of imports. TS-file requiring bundle in JS from one of my packages. But when a TS-file is requiring one of my bundles, and that bundle is requiring another bundle, it fails, since you guys don't apply that paths unless the file requiring is a TypeScript files. This is really just a monorepo-issue, albeit still an issue.

I can definitely try to point to the source. But that might not work in our CI-environment. And since you guys are removing support for TS_NODE_PROJECT I can't use different tsconfig.json for different environments.

Hopefully we can use tsconfig-paths for this though. I think the idea is to always have the paths available in the process.

[tytusplanck-8451]

@dgozman: Yeah, exactly. I point my paths to the dist-folder in each package so it mimics the behavior of being a regular installed dependency. But since they are not located in node_modules they can't be resolved without paths. This works great for one level of imports. TS-file requiring bundle in JS from one of my packages. But when a TS-file is requiring one of my bundles, and that bundle is requiring another bundle, it fails, since you guys don't apply that paths unless the file requiring is a TypeScript files. This is really just a monorepo-issue, albeit still an issue.

I can definitely try to point to the source. But that might not work in our CI-environment. And since you guys are removing support for TS_NODE_PROJECT I can't use different tsconfig.json for different environments.

Hopefully we can use tsconfig-paths for this though. I think the idea is to always have the paths available in the process.

I'm also in a monorepo environment where some tsconfig.json point to the dist vs. the actual source for testing/usage as a consumer or ourselves. Ideally, it would be awesome if we could pass tsconfigPath to the PlaywrightTestConfig option rather than assuming the path automatically.

@bellindj had a great example here.

[dgozman]

I discussed this issue with the team and we arrived to the following:

• Removing TS_NODE_XXX env variables support, because we are not following ts-node semantics.
• Do not respect tsconfig for javascript files as we do today.
• Recommend a separate tsconfig.json in the tests directory for any test-specific path mappings.

Overall, Playwright assumes that each file is either:

• A typescript source. In this case Playwright transforms it and respects tsconfig.
• A javascript file. In this case Playwright assumes it's ready to load as is.

If you have a question or need help with your setup, please share your repro and we'll try to help as we can. If you believe there is still a problem, please file a new issue with a repro and link to this one.