Enable PnP by default if within a PnP environment?

[arcanis]

This is a followup to #174. In this PR, we've implemented normalize-options.js as a way for third-party runtimes to hook into resolve by default and modify its default options.

It worked really well, and unlocked many packages - Babel being an important one. I just found out, however, that it doesn't work with one particular project: Next.js. For some reason, Next precompiles resolve and thus causes normalize-options.js to disappear, leaving no chance to the hook to override it.

There are a few options to solve that on their side, but I wonder: could/should resolve improve its support? Since 2018 a lot of new projects have adopted PnP, including enhanced-resolve from Webpack. Coupled to the fact that:

• This behaviour is in essence already enabled by default for everyone using PnP
• It doesn't affect at all people that don't use PnP
• The PnP API has been very stable for a while now
• PnP will be the default in Yarn 2+

How would you feel about adding the PnP logic into resolve itself if process.versions.pnp is detected (we would still have forceNodeResolution to prevent it, just like it is at the moment)?

[ljharb]

I'm not very comfortable with doing that; in general, precompiling in the way you imply next.js does brings with it risks, which include things like this. Have you filed an issue with next.js to fix its precompilation?

[arcanis]

Yep, along with a few candidate solutions: vercel/next.js#8659

Note that I don't think there's any urgency to do anything in resolve - I just felt it was a good time to start a discussion and consider options going forward! Keeping the current system is one 🙂

[ljharb]

It seems like the only options for resolve are:

1. keep things as they are, rely on consumers who precompile/bundle preserving the proper node semantics
2. add a process.versions.pnp check, which may cause an increase in bundle sizes to consumers who don't explicitly handle it by adding a process shim - typically things like this are handled with process.env, which bundlers all know how to minimally inline.

[arcanis]

We found an interesting edge case when porting the Babel repo to Yarn 2: they are using jest for their tests, and for some reason resolve wasn't working out of the box with PnP from within the test suites.

Turns out Jest is executing the module in its own sandbox, so the runtime never gets the chance to override normalize-options.js 🤔

[ljharb]

That seems like something that would certainly be easier if resolve auto-detected; but also is an inherent cost of running code in workers. Could the pnp runtime intercept the creation of workers, or their evaluation of code, and ensure that the right setup is done?

[arcanis]

I'm a bit cautious doing this, as it's generally quite intrusive and prone to break 🤔

I feel like the less we need to add a layer between Node and its users, the better I feel (for example that's part of the reason why we use NODE_OPTIONS to inject the runtime instead of overriding child_process and worker_threads).

[arcanis]

Another edge case: Yarn offers a way to 'symlink' a package to a specific folder on the disk through the use of the link: protocol (I think npm does something similar with file:):

{
  "dependencies": {
    "my-app": "link:./src"
  }
}

Since the PnP integration operates by telling resolve where is the root of the node_modules, it works for regular packages (because we take care of installing them in such a way that there is a node_modules prefix for each package, ie /cache/foo-1.0.0/node_modules/foo/...), but it cannot work for the link: protocol - there is just no node_modules component in the path hierarchy that we can return to resolve (ie /path/to/project/src).

[ljharb]

Hmm, I'm not clear on why it being a link means you can't provide a fake location for it. Can you elaborate?

[arcanis]

Imagine resolving foo/bar/hello. The resolve algorithm will get the node_modules paths, then try to resolve ./foo/bar/hello from them until it finds a matching one.

If foo is a package, the returned node_modules path is one located within the archive:
/project/cache/foo-1.0.0/node_modules

Then resolve takes this path, join it to ./foo/bar/hello, and finishes resolving that:
/project/cache/foo-1.0.0/node_modules/foo/bar/hello.js

Now imagine the same if foo is described with link:./src instead of being a package. What path can Yarn return that, when joined with ./foo/bar/hello, will yield /project/src/bar/hello? There is none, because resolve will always keep the dependency name in the path concatenation. So even if we were to return /project/src, it would just cause resolve to try out /project/src/foo/bar/hello - which doesn't exist.

[arcanis]

Fwiw, a generic fix for that would be for the paths settings (or at least the functor version) to return the path to the package sources rather than the path to the package n_m directory (ie the package source folder's dirname).

In this case the functor would return /project/cache/foo-1.0.0/node_modules/foo for the package version, and /project/src for the link version. The resolve algorithm would then concatenate it with the subpath ./bar/hello (rather than the full path ./foo/bar/hello), and it would work in both cases.

Unfortunately it's probably a breaking change... 🙁

[ljharb]

v2 is out in prerelease form; we can still get breaking changes in. To clarify: the change you're suggesting would be for the paths option function - which is currently invoked and returned here, to change how?

[arcanis]

I made a tentative PR: #205 - basically giving the hook the option to skip the path.join if it doesn't apply to a package.

If possible at all, I would really appreciate if the change could be made in a semver-minor release. Lots of packages are depending on resolve, and that would help compatibility in one fell swoop.

[ljharb]

Thanks; if it's nonbreaking then I'll definitely backport it to 1.x.

[martpie]

What is exactly the status on this? I am facing a really weird issue.

using resolve from my app works fine with PnP, but when I publish a package which use resolve, when my package gets executed, I get resolutions errors:

Error: Cannot find module 'next/head' from '/Users/pierre/dev/test-ntm-berry/.yarn/cache/next-transpile-modules-npm-5.0.0-beta.3-300d124630-a27b3b34e5.zip/node_modules/next-transpile-modules/src'
    at Function.resolveSync [as sync] (/Users/pierre/dev/test-ntm-berry/.yarn/cache/resolve-patch-46f4fba2f6-188d5167e8.zip/node_modules/resolve/lib/sync.js:90:15)

So it looks like when something in node_modules tries to use resolve, PnP module resolution fails. Did I miss something? Is there a flag I need to pass to resolve.sync() to enable PnP resolution?

[martpie]

Ok, for anyone facing the same issue than me, make sure basedir is process.cwd() and not __dirname

[ljharb]

@arcanix given this is in v1.15+, and since i'd prefer not to hardcode PnP stuff in this package, it seems like this can be closed. lmk if it needs to be reopened.