[rush] PNPM doesn't use global store

[Js-Brecht]

Is this a feature or a bug?

• Feature
• Bug
  - [x] Other: Issue with behavior

Please describe the behavior.

PNPM does not use the global store location. Instead, it creates a new store in common\temp\pnpm-store. This essentially defeats one of the best features of PNPM; having a global store saves a lot of time, because packages do not have to be downloaded for every project... As it is now, in order to start a new spfx repo using rush, I have to download 1300+ packages, even though I already have them installed on my system. I understand these packages will be shared with all of the projects in the repo, but that is already the case when using PNPM. Can't Rush manage dependency versions and such across the repo without needing its own store??

I have already had to wipe out the common\temp directory a couple times just because Rush would throw an error after adding a new project (about not having a dependency key), so each time that common\temp\pnpm-store gets deleted, I have to download 1300+ packages again.

Why does rush need a brand new store, and is there a way to disable that, without hardcoding the store location into the configuration? Hardcoding a path to the store is not an option for me. I work across several different machines, and the store is not always in the same location, since I use Linux and Windows, and the development environment is in different locations/partitions on each machine.

[iclanton]

Rush puts its store inside the repo folder to make sure everything an install/build job creates on disk can be cleaned up by simply deleting the repo folder. This is important for ensuring that CI machines that don't get wiped between builds don't run out of disk space.

I totally understand your scenario, though. It seems like making this behavior different for the local dev scenario makes sense. We'll need to design this, though.

@Js-Brecht - From a user's perspective, how would you expect this to be configured?

[Js-Brecht]

Ah, I understand the reasoning now.

@iclanton To me, it would make sense to have a pnpmStore next to pnpmVersion in rush.json. Maybe a couple of options:

1. rush
   • Create store the rush way, in the repo
2. pnpm
   • Let pnpm use its own store, however it is configured.
3. /absolute/path/to/store --or-- ./relative/path/to/store
   • Let the user configure their own store location

I only included number 3 to remain consistent with pnpm, which allows you to specify your own store location on the command line. Could let it use the rush .npmrc instead, since you can also configure it there.

[Jabher]

I also encountered this.
I would appreciate greatly if some kind of global store is accessible between builds, e.g. in /tmp/pnpm-store, which is expected to be flushed someday and so only used for speedup purposes.

[Js-Brecht]

FWIW, I wound up hacking out the line in @microsoft/rush-lib that sets the --store cli parameter. Not a very permanent solution, but it seems to be working without any problem. I haven't really delved too deeply into all of the different functionality in Rush, so I'm kind of curious to see if it will cause any odd issues.

[octogonz]

It should be fine.

Would someone want to implement this as a configurable option? I feel like it's a pretty easy work item. We just need to decide how it is specified. For example as an environment variable or as a rush.json setting.

[Js-Brecht]

@octogonz I wouldn't mind, when I have a little free time in the next couple days.

I still like the idea of using a rush.json setting; makes it a little more obvious that there's a choice about how to handle the store. I'd probably design it just like I described before.

[Js-Brecht]

Could be a property under pnpmOptions called store.

Property could be undefined, or a string that matches pnpm, a relative path (^\.[\\/]), or an absolute path (^[\\/]).

• If undefined, uses the rush store location
• If string matches pnpm, don't define a store location at all (as a bonus, pnpm should use .npmrc, if it's defined there)
• If string is a path... use that path.

[octogonz]

Some more thoughts about the design:

• I like pnpmStore better than store since it's more clear, particularly since we support multiple package managers.

• The string pnpm looks like a file path. Maybe separate it into two settings pnpmStore and pnpmStorePath (which isn't used if the pnpmStore kind is global or common)?

• /absolute/path/to/store probably wouldn't make sense in rush.json, as it would presumably be different on each computer. But absolute paths could be specified as a command-line parameter for rush update and/or an environment variable (our CLI library makes this easy)

• We try to avoid relying on settings from .npmrc, since that file format is vaguely specified, has very little error checking, and isn't handled consistently by different package managers

• What if the setting changes? For example, suppose the store was set to one thing when I ran rush update, but then I edit rush.json and run rush update again. I believe PNPM will complain about this. So Rush should detect that the setting has changed, and refuse to run, e.g. suggesting to rush update --purge to start over. This means Rush needs to remember where the actual store is (independent of the setting). Maybe it can go in last-install.flag

[Js-Brecht]

• I like pnpmStore better than store since it's more clear, particularly since we support multiple package managers.

Is the distinction necessary, if it's pnpmOptions.store? I was under the impression that those were essentially replications of the command line flags passed to pnpm. Not that I'm opposed; just curious.

Or do you mean put pnpmStore next to pnpmVersion, instead of under pnpmOptions?

• The string pnpm looks like a file path. Maybe separate it into two settings pnpmStore and pnpmStorePath (which isn't used if the pnpmStore kind is global or common)?

Works for me. I assume common means use rush's method?

• /absolute/path/to/store probably wouldn't make sense in rush.json, as it would presumably be different on each computer. But absolute paths could be specified as a command-line parameter for rush update and/or an environment variable (our CLI library makes this easy)

I assume that a cli parameter/environment variable will override the store path, no matter if pnpmStore is set to global or common.

While on the subject of paths, I have one question: Where should relative paths be relative to? I assume the default, or user-defined, temp directory, where pnpm is actually run? I only ask because it might be worth documenting that; if the setting is added to rush.json, people might be led to believe that the path will be relative to the root of the repository.

So, considering workspace located at /rush, if I define a store path of ./pnpm-store, the store will ultimately wind up at /rush/config/temp/pnpm-store, instead of /rush/pnpm-store. In other words, drop the relative path exactly as is onto the command line, but make sure the user knows where the store is actually going to land.
^ This would be the least complicated.

• We try to avoid relying on settings from .npmrc, since that file format is vaguely specified, has very little error checking, and isn't handled consistently by different package managers

Using .npmrc would just be a side-effect. I can't remember if there's a CLI argument to ignore the .npmrc, but I don't think there is.

1. Could let pnpm operate how it wants to if pnpmStore is global (which would result in using the .npmrc, if the option is defined there),
2. or we could try to find the system's pnpm store path, and define it on the command line so that the .npmrc is ignored.

Number 1 sounds like the best option to me. If the user put the store path in the .npmrc, I would assume that's what they want to happen. Number 2 could be a bit more complicated.

• What if the setting changes? For example, suppose the store was set to one thing when I ran rush update, but then I edit rush.json and run rush update again. I believe PNPM will complain about this. So Rush should detect that the setting has changed, and refuse to run, e.g. suggesting to rush update --purge to start over. This means Rush needs to remember where the actual store is (independent of the setting). Maybe it can go in last-install.flag

Wouldn't rush update modify pnpm-lock.yaml? Would that be desired behavior, or does it even matter when using rush? I guess that if specific versions are required, then their version specifier would be specific, or that version should be under preferredVersions? 🤷‍♂️

[octogonz]

Is the distinction necessary, if it's pnpmOptions.store? I was under the impression that those were essentially replications of the command line flags passed to pnpm. Not that I'm opposed; just curious.

I tend to prefer names like pnpmStore that are unambiguous when you talk about them in tech support, or search for them in docs/issues/code. For example when we eventually move pnpmVersion under pnpmOptions I would prefer not to rename it to version, because it's less searchable. You'd have to qualify it as pnpmOptions.version which perhaps still won't always be found by some searches.

But I don't feel super strongly about this; it's just something to consider.

Or do you mean put pnpmStore next to pnpmVersion, instead of under pnpmOptions?

I was thinking something maybe like this:

"pnpmOptions": {
  /**
   * Specifies the location of the PNPM store.  There are three possible values:
   *
   * - "local" - use the "common/temp/pnpm-store" folder in the current repo
   * - "global" - use PNPM's global store, which has the benefit of being shared
   *    across multiple repo folders, but the disadvantage of less isolation for builds
   *    (e.g. bugs or incompatibilities when two repos use different releases of PNPM)
   * - "path" - use the location specified by "pnpmStorePath"
   *
   * The default value is "local".
   */
  "pnpmStore": "local",

  /**
   * This setting is only used when the "pnpmStore" setting is "path".  It specifies a custom
   * path for the PNPM store, resolved relative to the root folder containing rush.json.
   *
   * It is not recommended to store an absolute path in the rush.json file.
   * For machine-specific overrides, use the RUSH_PNPM_STORE_PATH environment
   * variable instead.
   */
  "pnpmStorePath": "common/temp/something-else"
}

While on the subject of paths, I have one question: Where should relative paths be relative to? I assume the default, or user-defined, temp directory, where pnpm is actually run? I only ask because it might be worth documenting that; if the setting is added to rush.json, people might be led to believe that the path will be relative to the root of the repository.

I agree that people would generally assume it's resolved relative to the rush.json folder. But like you say, maybe it's more useful to resolve relative to the temp folder, which BTW can be overridden using RUSH_TEMP_FOLDER.

If you believe that potentially people would legitimately want more one than base folder to resolve against, we do have a convention of supporting tokens in paths. With this design you could do <repoRoot>/foo or <rushTemp>/pnpm-store or <pnpmGlobal>. This would eliminate the need for two settings. However, it's more involved to implement, and a bit harder for people to understand. It feels maybe like overengineering, given that so far relatively few people have asked to customize the temp folder. Up to you...

Number 1 sounds like the best option to me. If the user put the store path in the .npmrc, I would assume that's what they want to happen. Number 2 could be a bit more complicated.

That seems reasonable.

Wouldn't rush update modify pnpm-lock.yaml? Would that be desired behavior, or does it even matter when using rush? I guess that if specific versions are required, then their version specifier would be specific, or that version should be under preferredVersions? 🤷‍♂

I was referring to this issue:

C:\> md test
C:\test> npm init
C:\test> pnpm install jquery
C:\test> pnpm install  --store C:\custom

 ERROR  Unexpected store used for installation

expected: C:\.pnpm-store\2
actual: C:\custom\2

If you want to use the new store, run the same command with the --force parameter.

Also, I believe --force is telling PNPM to trust that the two stores are consistent with each other, which maybe is not always true. Seems safer to do rush install --purge when switching stores. (This could be my own paranoia, but to be fair this paranoia is based on years of supporting a very active monorepo where every weird little glitch in the system could turn into an afternoon of investigation heheh.)

[octogonz]

I noticed that already PNPM writes the store choice into a file:

node_modules/.modules.yaml

hoistedAliases: {}
included:
  dependencies: true
  devDependencies: true
  optionalDependencies: true
independentLeaves: false
layoutVersion: 2
packageManager: pnpm@3.5.0
pendingBuilds: []
registries:
  default: 'https://registry.npmjs.org/'
shamefullyFlatten: false
skipped: []
store: 'D:\.pnpm-store\2'  # <-----------------

Maybe it's safe for Rush to rely on that.

[Js-Brecht]

It specifies a custom path for the PNPM store, resolved relative to the root folder containing rush.json.
...
"pnpmStorePath": "common/temp/something-else"

This does feel more natural, instead of trying to shift context when you're setting up the store path. It shouldn't be hard to resolve relative to the root, and since it is less of a mental hurdle, that's the route I'll take.

we do have a convention of supporting tokens in paths. ... It feels maybe like overengineering

I agree, that does seem a bit much; better to keep it simple. This could always be added down the road if it seems like there's demand for it. If the user decides they want to put their store in the temp folder (and let's say they've explicitly overridden the temp folder), then they can just change pnpmStore to local, or include an environment variable like they already did for the temp folder.

Seems safer to do rush install --purge when switching stores.

rush install --purge is kind of what I was leading up to, as opposed to rush update --purge.

I think that it could be handled internally, by setting the --force switch for pnpm if the current store directory doesn't match the previous. That will rebuild the node_modules directory, which is basically the goal anyway, yeah?

Changing store locations using --force

Using a custom store:

PS D:\temp> pnpm install --store .\pnpm-store @types/node
PS D:\temp> cat .\node_modules\.modules.yaml
...
store: 'D:\temp\pnpm-store\2'
...

PS D:\temp> fsutil hardlink list .\node_modules\@types\node\package.json
\temp\pnpm-store\2\registry.npmjs.org\@types\node\12.12.8\node_modules\@types\node\package.json
\temp\node_modules\.pnpm\registry.npmjs.org\@types\node\12.12.8\node_modules\@types\node\package.json

---

Changing to a different store errors as expected:

PS D:\temp> pnpm install @types/node
 ERROR  Unexpected store location

The dependencies at "D:\temp\node_modules" are currently linked from the store at "D:\temp\pnpm-store\2".

pnpm now wants to use the store at "D:\.pnpm-store\2" to link dependencies.

If you want to use the new store location, reinstall your dependencies with "pnpm install --force".

You may change the global store location by running "pnpm config set store-dir <dir>".
  (This error may happen if the node_modules was installed with a different major version of pnpm)

Notice the output here:

PS D:\temp> pnpm install @types/node --force
 WARN  using --force I sure hope you know what you are doing
Recreating D:\temp\node_modules # <------------------------------ 
Packages: +1
+
Resolving: total 1, reused 0, downloaded 1, done

dependencies:
+ @types/node 12.12.8

PS D:\temp> cat .\node_modules\.modules.yaml
...
store: 'D:\.pnpm-store\2' # Changed from 'D:\temp\pnpm-store\2'
...

And then checking the hardlinks again, to make sure they are linked to the correct store.

PS D:\temp> fsutil hardlink list .\node_modules\@types\node\package.json
\.pnpm-store\2\registry.npmjs.org\@types\node\12.12.8\node_modules\@types\node\package.json
\temp\node_modules\.pnpm\registry.npmjs.org\@types\node\12.12.8\node_modules\@types\node\package.json

---

Two caveats:

1. That would mean the old store would linger, which isn't necessarily a bad thing, IMO (less downloads in the future, for example, if they decided to switch back). If it's in the temp folder, it's going to be purged eventually anyway; if it's not, then the user knows where they put it, and why, and they can delete it if they want to.
2. This would make changing stores a bit less explicit. I can see how that could make issues harder to track down, if the user wasn't fully cognizant of the changes that were being made. If they are made to run rush install --purge before they can finish the install, and issues occur, they will be more aware about what could have caused them.

Feels like an executive decision to me, since I'm more inclined to streamline the process. If making it more explicit is what you want, then that is what I'll do.

[octogonz]

Feels like an executive decision to me, since I'm more inclined to streamline the process. If making it more explicit is what you want, then that is what I'll do.

I'm fine with keeping it simple. The underlying requirements I had in mind were:

• rush install / rush update ideally should not fail with PNPM instructing the user to run a PNPM command. It's confusing to have two different tools telling you what to do. Also, for basic everyday tasks, we try to provide a consistent user experience in every repo, regardless of package manager choice. If some special action needs to be taken (e.g. --purge), it's usually a better experience for this to be explained by Rush and solved using the Rush command line.

• Mainly I wanted to avoid mistakes or corrupted states. For example, suppose someone runs rush install from a shell with RUSH_PNPM_STORE_PATH, but then they run rush add from another shell without RUSH_PNPM_STORE_PATH. If we can safely switch stores in a way that's 100% guaranteed to work correctly, I'd be okay with Rush automatically invoking pnpm --force and printing a notice to the console. But if there is some risk of corruption or possibility that they made a mistake, then we should take the user through an explicit step of of switching stores. It doesn't need to be rush install --purge; maybe you're required to run rush update --update-pnpm-store or even just rush update first, etc. Just something to confirm the intent.

As far as how we satisfy these requirements, you'll probably have a better sense from trying out the commands. I think we have enough to proceed with an implementation. 👍

Let us know if you need any help debugging Rush or figuring out how to add config options. Thanks @Js-Brecht !

[Js-Brecht]

@octogonz hey, sorry it's taken me so long to get this done. I was just able to make time for it starting yesterday. PR is #1657

[octogonz]

🎈 This feature was published with Rush 5.21.0. Thanks @Js-Brecht for implementing it!