Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

UpstartMPotnick
Copy link

@UpstartMPotnick UpstartMPotnick commented Oct 1, 2024

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

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

@UpstartMPotnick UpstartMPotnick force-pushed the mpotnick/more-specific-pnpm-paths branch from bc96598 to df49615 Compare October 2, 2024 16:16
@UpstartMPotnick UpstartMPotnick changed the title fix(pnpm): uses slugified scope and name for hardlink package location fix(pnpm): uses actual scope and name for hardlink package location Oct 2, 2024
@UpstartMPotnick UpstartMPotnick force-pushed the mpotnick/more-specific-pnpm-paths branch 3 times, most recently from 6a31e88 to 4502482 Compare October 3, 2024 20:04
@UpstartMPotnick UpstartMPotnick changed the title fix(pnpm): uses actual scope and name for hardlink package location fix(plugin-pnpm): uses actual scope and name for hardlink package location Oct 4, 2024
@UpstartMPotnick UpstartMPotnick force-pushed the mpotnick/more-specific-pnpm-paths branch 2 times, most recently from 2bdeba9 to 8c16144 Compare October 11, 2024 14:46
@arcanis
Copy link
Member

arcanis commented Oct 15, 2024

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

@UpstartMPotnick
Copy link
Author

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.

@arcanis
Copy link
Member

arcanis commented Nov 7, 2024

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.

@UpstartMPotnick UpstartMPotnick force-pushed the mpotnick/more-specific-pnpm-paths branch from 8c16144 to b346155 Compare November 7, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: pnpm linker does not preserve name of package directory
2 participants