berry icon indicating copy to clipboard operation
berry copied to clipboard

fix(plugin-pnpm): uses actual scope and name for hardlink package location

Open UpstartMPotnick opened this issue 1 year ago • 4 comments

What's the problem this PR addresses?

The current implementation of the pnpm linker stores package hardlinks within a nested structure consisting of a hashed directory plus an arbitrary package directory. e.g. .store/lodash-npm-4.17.21-6382451519/package. The appended package path is a literal string in the PnpmLinker and (I assume) originated from the standard yarn|npm pack output filename.

Rarely, package internals might introspect the filesystem to apply some logic / constraints. Notably, Next.js has a specific check to guarantee modules are loaded relative to a next/ directory. This check fails because the hardlink exists inside of the package directory.

Conversely, when using pnpm directly, this check is successful because it places these hardlinks into a nested structure that includes the dependency name, e.g.: .pnpm/[email protected][email protected][email protected]/node_modules/next

Resolves #6269

How did you fix it?

This PR updates the getPackagePaths method to leverage the existing structUtils.stringifyIdent method to generate the nested hardlink directory instead of using the literal package string. This more closely aligns with the naming conventions of pnpm (and is also how the pnpm linker plugin behaves in Yarn v3).

Checklist

  • [x] I have set the packages that need to be released for my changes to be effective.
  • [x] I will check that all automated PR checks pass before the PR gets reviewed.

UpstartMPotnick avatar Oct 01 '24 23:10 UpstartMPotnick

Thanks! Can you also add a test for this behaviour?

arcanis avatar Oct 15 '24 22:10 arcanis

Thanks! Can you also add a test for this behaviour?

Does it need a specific test? The location of the package seems to be an implementation detail; the literal path isn't itself tested, but there are existing integration tests (e.g. basic.test.ts) for each linker that already seem to validate the behavior. Likewise, the package install path for the pnp linker isn't directly tested either, as far as I can tell, but relies on the same integration tests.

UpstartMPotnick avatar Nov 06 '24 14:11 UpstartMPotnick

I think so; since your PR is intended to fix a specific obscure behaviour, having a test is quite important (otherwise noone will remember about it in a year time):

Rarely, package internals might introspect the filesystem to apply some logic / constraints. Notably, Next.js has a specific check to guarantee modules are loaded relative to a next/ directory. This check fails because the hardlink exists inside of the package directory.

arcanis avatar Nov 07 '24 14:11 arcanis

what's the status of this PR? I could really use this enhancement for my NextJS projects that behave more reliably on pnpm's linker (at least when I'm using the real thing)

kherock avatar Apr 11 '25 14:04 kherock