PNP tsconfig references resolution fix

[sushruth]

Fixes yarnpkg/berry#3231

Also slightly makes it easier for nx repositories to adopt yarn 3 (nrwl/nx#2386)

CC: @arcanis

[merceyz]

cc @andreialecu as the author of that patch

[sushruth]

@andreialecu your review on this would be much appreciated. This is trying to address breaking scenarios for monorepos where tsconfig files and references are deep within the root of the workspace.

Thank you.

[andreialecu]

Interesting. I'm using yarn 3 and project references but I don't remember noticing anything wrong.

How does the problem manifest exactly?

[andreialecu]

Hey @sushruth I just took this for a test spin.

Unfortunately all it does is to essentially undo #4 because all paths going through that function are absolute.

You can verify in my original repro at: https://github.com/andreialecu/repro-yarn-pnp-composite which I just updated to yarn 3.

Run ./test-refs.sh or look inside packages/app/src/index.ts. It should NOT show this:

But with this patch, the behavior above happens.

---

To clarify, when did you first see this problem occuring? Did the behavior or pnpApi.resolveToUnqualified() change recently? Perhaps the problem lies there?

[merceyz]

The problem appears to be that the current patch resolves the root of the locator but in the repro provided in yarnpkg/berry#3231 the reference isn't pointing to a folder (workspace root) so we're effectively turning the reference ./tsconfig.app.json into ../../

[sushruth]

@andreialecu Those errors seem to be expected behavior in yarn 2 with TS 4.3.5 - https://github.com/sushruth/repro-yarn-pnp-composite.

Edit 1: P.S. - This behavior (Cannot find module "lib") is also observed when running yarn tsc -p packages/app/tsconfig.json and also observed in vscode when yarn is not involved at all and npm is used to install dependencies. -

packages/app/src/index.ts:1:21 - error TS2307: Cannot find module 'lib' or its corresponding type declarations.

1 import { Foo } from "lib";
                      ~~~~~
packages/app/src/index.ts:2:27 - error TS2307: Cannot find module 'shared' or its corresponding type declarations.

2 import { FooShared } from "shared";
                            ~~~~~~~~
packages/app/src/index.ts:3:24 - error TS2307: Cannot find module 'lib2' or its corresponding type declarations.

3 import { Shared } from "lib2";
                         ~~~~~~
Found 3 errors.

I may be wrong here but seems to me like being able to reference types in lib folder (namespaces, module declarations, globals, etc) does not seem to mean being able to import anything from that folder via the import name "lib". For that to happen, we may need to use "paths" tsconfig option.

Edit 2: This makes me think if the updateReferences function needed any update at all. Any thoughts on this, @merceyz / @arcanis / @andreialecu ?

[andreialecu]

@andreialecu Those errors seem to be expected behavior in yarn 2 with TS 4.3.5 - sushruth/repro-yarn-pnp-composite.

Not sure I understand what you mean. The behavior is not expected, because it works properly without PnP, or when virtuals are not involved, so it should definitely work. It's the most idiomatic way to use project references as well. The patch I made in #4 fixed that.

The change made by this PR simply undoes it which incidentally makes your scenario work.

I may be wrong here but seems to me like being able to reference types in lib folder (namespaces, module declarations, globals, etc) does not seem to mean being able to import anything from that folder via the import name "lib". For that to happen, we may need to use "paths" tsconfig option.

paths should not really be used with project references and yarn tsc -p packages/app/tsconfig.json doesn't make sense in the context of project references either. The only correct way to build is with --build or tsc -b. Which leads me to the following question:

---

I'm a bit confused by how the setup in your repro translates into a real world scenario because I'm not sure why your tsconfig would look like that in practice. It seems a bit odd. :)

Even so though, I see that your repro works properly with node_modules, so there's definitely a bug in there somewhere, but this fix isn't the correct one.

[andreialecu]

Can you clarify if this is a regression and when did it start appearing? Did yarn 3 introduce it or TS 4.3.5? What's the last combo of versions when it was working?

[sushruth]

@andreialecu Turns out I was wrong. I just realized that I had completely ignored the fact that your repro repo is a yarn workspace monorepo. Now I realize what might be the actual issue that was breaking my scenario (mostly for all nx repos for reference)

I think it might actually be that the current typescript-patch is not considering the scenario when references are files without a workspace and not workspace folders with tsconfig.json in them.

Can you clarify if this is a regression and when did it start appearing? Did yarn 3 introduce it or TS 4.3.5? What's the last combo of versions when it was working?

basically upgrading to the latest typescript-patch broke all nx monorepos due to this issue. It was not necessarily introduced in TS or Yarn3.

I am now working on a different solution for this issue. Thanks @andreialecu. Reverting this PR to draft.

EDIT: The new proposed solution seems to work for all these repro repos -

1. https://github.com/sushruth/berry-issue - representing most nx repos
2. https://github.com/andreialecu/repro-yarn-pnp-composite - representing yarn workspace use case
3. https://github.com/sushruth/repro-yarn-pnp-composite - It also works in the off-scenario that the root workspace is also a published package and needs to be added to tsconfig references.

@andreialecu I am opening this PR up for review once again. Please take a look. Thank you.

[merceyz]

I think it might actually be that the current typescript-patch is not considering the scenario when references are files without a workspace and not workspace folders with tsconfig.json in them.

Exactly, that's what I was hinting at in #10 (comment). The patch needs to be changed to preserve the part of the path that comes after the packageLocation but still run through the same code path

[sushruth]

@merceyz that sounds right. One way I though about this is - any path that directly resolved to be from the root workspace would mean PnP may not need to handle the virtual directory. I made a change to reflect that. What do you think about the current diff of this PR?

[sushruth]

@andreialecu pinging for your attention on latest update to this PR.

[sushruth]

@merceyz @andreialecu @arcanis pinging to bring attention to this

[arcanis]

Hey! I'm sorry I'm a little lost 😅 can you make a quick summary of why this is needed? I'm worried about adding a special case just for workspace:., I'm not sure why we need that.

[merceyz]

What I said in #10 (comment) and #10 (comment) still stands, I pushed the generic fix here merceyz@3f4e151, will update yarnpkg/berry#3297 to include it

[sushruth]

Closing in favor of @merceyz's PRs. Thank you @merceyz !