Conversation
gluxon
commented
Mar 10, 2025
| peer: opts.savePeer, | ||
| pruneLockfileImporters: ((opts.ignoredPackages == null) || opts.ignoredPackages.size === 0) && | ||
| pkgs.length === allProjects.length, | ||
| pruneLockfileImporters: opts.pruneLockfileImporters ?? |
Member
Author
There was a problem hiding this comment.
One other option: We could name the publicly exposed option produceFilteredWantedLockfile and have that value imply the internal pruneLockfileImporters value here.
The idea of produceFilteredWantedLockfile would be to intentionally create a wanted lockfile that omits certain importers, which is the goal of pnpm deploy. There could probably be a better name for produceFilteredWantedLockfile if we like that idea.
zkochan
approved these changes
Mar 11, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Upon updating
allProjectsGraphto always contain all projects (#9259), only 1 test began failing:Explanation
This test caught a real problem. When
project-1depends onproject-2, the workspace dependency was no longer getting injected into thedeploydirectory.This is because when
pnpm deployis ran withnode-linker=hoisted, we add all workspace packages as an argument to thehoistfunction.pnpm/pkg-manager/real-hoist/src/index.ts
Lines 51 to 66 in a76da0c
This hoists all workspace packages into the original root
node_modulesinstead of the injected deploy directory. As a result, pnpm installs them in their normal location and never in the deploy directory.Why did the
allProjectsGraphrefactor cause this?Before this change, the
allProjectsGraphonly contained the root project and the selected project for deploy. This meantlockfile.importersonly contained those 2 projects. WhenallProjectsGraphwas updated to be all projects, thelockfile.importersobject contained all projects.Here's this loop before the
allProjectsGraphrefactor:Here's this same code path after my
allProjectsGraphchange. Note howproject-2andproject-3are now included.Changes
I've brainstormed a few fixes, but I think the best one is to pass down
pruneLockfileImportersfrom thedeployfunction. This results inlockfile.importersonly containing the deployed project.Workspace packages that the deployed project depends on are still recursively included through the
file:protocol, but they're no longer seen as importers from the perspective of a deployed install. I think this makes sense, especially since the lockfile entry for the these importers are always empty objects in this scenario.Alternatives
We could also pass an
importersToHoistarray to@pnpm/real-hoistand iterate over that instead oflockfile.importers, but this felt hackier. It also doesn't fix the wanted lockfile for deploy. There would still be empty unused importers indeploy/node_modules/.pnpm/lock.yaml.I also explored updating the
modulesDirused to install hoisted workspace packages here:pnpm/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts
Line 108 in a76da0c
To respect
opts.modulesDirlike we do for external packages slightly above:pnpm/pkg-manager/headless/src/lockfileToHoistedDepGraph.ts
Line 87 in a76da0c
However, this would be a very large refactor and potentially change the behavior of
node-linker=hoisted.