fix(pnp): resolve the virtual path of the pnpapi instead of the issuer

[merceyz]

What's the problem this PR addresses?

findApiPathFor passes the search path through VirtualFS.resolveVirtual instead of the .pnp.* file which causes it to not find the pnpapi when using the portal: protocol into non-pnp projects and when using the global cache.

Fixing this will allow me to add a e2e test to gridsome/gridsome#1313

How did you fix it?

Pass the found .pnp.* path through VirtualFS.resolveVirtual instead of the search path.

Checklist

• I have read the Contributing Guide.
• I have set the packages that need to be released for my changes to be effective.
• I have verified that all automated PR checks pass.

[arcanis]

I feel like it's still a problem, because it makes a double lookup for virtual paths - so if the portal'd project is a PnP project, it'll have a PnP file, and thus findApiPathFor will keep incorrectly returning the external one 🤔

Instead, it seems like the ideal would be to only run the lookup based on the virtual path. I think the resolveVirtual is only done because of a weird case where the lookup finds something like /project/$$virtual/0/abc123/.pnp.js as the PnP file, but I don't remember the details ...

[merceyz]

because it makes a double lookup for virtual paths

Only if the target isn't a PnP project

so if the portal'd project is a PnP project, it'll have a PnP file, and thus findApiPathFor will keep incorrectly returning the external one 🤔. Instead, it seems like the ideal would be to only run the lookup based on the virtual path

It would find the external one anyways

Instead, it seems like the ideal would be to only run the lookup based on the virtual path. I think the resolveVirtual is only done because of a weird case where the lookup finds something like /project/$$virtual/0/abc123/.pnp.js as the PnP file, but I don't remember the details ...

I agree, reading over the original issue (#1524 (comment)) it seems the conclusion @bgotink came to was that we needed to resolve the virtual path of the pnpapi we find, not the path we search, making that change fixes the issue i'm having with portals and prevents double searches

[clemyan]

I have been thinking about this for some time. The merged solution is better than nothing but I have some concerns/comments. (It is totally possible that I misunderstood some things and if so please point them out)

Problem statement

First of all, let's define the problem we are trying to fix with this PR. Let's say we need to find the PnP API for the virtual path

/path/to/project1/.yarn/$$virtual/<slug>/1/project2/.yarn/$$virtual/<slug>/1/project3/package.json

The behavior before this PR was that path would be resolved to /path/to/project3/package.json and a PnP API would be searched from that path. This allows project3's PnP API to "take control" of paths under /path/to/project3 if project3 is a PnP project. As far as I can see, this behavior was introduced by the combination of #1524 and #630. (@merceyz originally said multiple PnP APIs enables CRA?) The problem is, if project3 is not a PnP project, then the search just fails.

As far as I can see, the correct behavior would be attempting to find a PnP API at all of these paths (among others), in high to low precedence order:

/path/to/project3/.pnp.js
/path/to/project2/.pnp.js
/path/to/project1/.pnp.js

For most common cases, this PR does solve this problem. But I have 2 concerns.

Caching

The search algorithm caches any path it has searched before. The merged solution sometimes ignores the cache.

In particular, say package-a is virtualized twice with different hashes

/path/to/project1/.yarn/$$virtual/package-a-aaaaaaaaaa/1/project2/pacakge.json
/path/to/project1/.yarn/$$virtual/package-a-bbbbbbbbbb/1/project2/pacakge.json

These two virtual paths point to the same location on the actual fs, so they should be and was cached under the same key. However, the merged solution breaks that.

Edge cases

All of the above assumes the virtualFolder config is the default ./.yarn/$$virtual. However, virtualFolder can be set to any relative or absolute path. Say it is set to /$$virtual, then the 3-project case at the top of this comment would be virtualized to

/$$virtual/<slug>/0/path/to/project3/package.json

in which case there is no way for the PnP API search to recover the paths of project2 and project1.

I think the configurability of virtualFolder exists because virtualization was achieved by creating symlinks under the virtual folder. Now that virtual paths are literally virtual (does not exists in any form on the actual fs), I don't think this configuration is actually useful. On the other hand, this configurability is actually preventing a robust solution to this problem.

Conclusion

Given that I don't think a robust solution can be implemented without removing the virtualFolder config (a semver-major change), I think we settle for the current solution and revisit this for Yarn 3.

[arcanis]

@merceyz Btw, I just noticed that the PR didn't add a test - I think it's something that should be covered, as we already regressed once. Do you have a small testcase we could add to pnp.test.js?