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): various improvements #3681

Merged
merged 9 commits into from
Nov 9, 2021
Merged

Conversation

paul-soporan
Copy link
Member

@paul-soporan paul-soporan commented Oct 30, 2021

What's the problem this PR addresses?

  • The PnpmLinker didn't install transitive dependencies in such a way that they can require themselves
  • The PnpmLinker didn't support running binaries of soft links
  • The PnpmLinker didn't remove the scope folder (e.g. node_modules/@yarnpkg) after removing a scoped dependency
  • The PnpmLinker didn't remove folders that looked like scopes

How did you fix it?

  • Made it copy the package sources to node_modules/.store/<hash>/node_modules/<ident> and use that as the symlink source so that self-references work. There is a special edge-case though: when a package depends on a different version of itself. Both pnp and nm get that right (though pnpm doesn't), so I made it skip generating self-references in that case (i.e. made it keep copying the sources to node_modules/.store/<hash> so that the dependency can be put inside node_modules/.store/<hash>/node_modules/<ident>. The sources will now be copied to node_modules/.store/<hash>/node_modules/<ident> and dependencies will be symlinked to node_modules/.store/<hash>/node_modules/<ident>/node_modules.
  • Made it persist packageLocations (now renamed to pathByLocator) inside the install state and made findPackageLocation query that
  • Made it remove those
  • Made it remove those

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.

@edvardchen
Copy link
Contributor

I saw this PR when I was going to submit my own PR. It seems that all issues I encountered will be fixed by this PR. Look forward to it!

@arcanis arcanis merged commit caefa4a into master Nov 9, 2021
@arcanis arcanis deleted the paul/fix/pnpm-self-requires branch November 9, 2021 16:16
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.

None yet

3 participants