Using pnpm

[aminya]

pnpm is much faster than npm or yarn. We should consider using that to speed up the build process. It is compatible with npm, so switching to it is just a matter of changing npm to pnpm in the scripts that install something.

https://pnpm.js.org/en/benchmark

We can even add pnpm support to apm.

[DeeDeeG]

Mixed feelings:

First of all, neat idea.

Second, I am concerned this would be moved forward too quickly without being vetted. If we test it enough and know it inside and out, and can clearly prove that various points of different behavior from vanilla npm are going to be okay, then sure.

• I think we should avoid using pnpm's pnpm-lock.yaml if pnpm will let us. (If it will let us just keep using package-lock.json as a lockfile, but still install faster in CI.) Remember, we maintain all diff from upstream ourselves. One less custom piece of different tech, one less bit of maintenance/bugfix/chore work to worry about. We don't want to spend time with this fork doing that kind of work, IMO.

• Strict. A package can access only dependencies that are specified in its package.json.

  • Will this break any packages? Spend the time to make sure it doesn't. Packages might be relying npm's looser behavior here.

Regarding apm: neat idea, but I don't want to actually do this.

Let's not do this in apm bundled with Atom unless it has been supremely well-tested. Core infrastructure like apm is responsible for building Atom's bundled packages, and any user/community packages. And speaking as someone who's patched it, it's a pile of spaghetti. Surprisingly easy to break.

apm is an elaborate wrapper around vanilla npm. Using it with pnpm instead could take a large rewrite of the codebase in order to still work as designed. Any quirks in implementation of either npm or pnpm could derail this. Would not be surprised if apm requires features of npm that aren't in pnpm. Not looking forward to verifying that none of apm breaks with pnpm, would be quite surprised if so.

Given that apm is a pile of spaghetti that mostly works, trying to rewrite it is going to be unpleasant. Better to make it simpler and more standard than try to improve it in a non-standard way.

[DeeDeeG]

I will be happy with this if:

• It installs faster

But I will be unhappy with this if:

• It breaks things in Atom
• It makes us heavily patch Atom, or fork and patch other areas of the Atom ecosystem, to work with pnpm

[DeeDeeG]

Steps to switch to pnpm (assuming no bumps in the road or incompatibilities, quirks, etc):

• CI: Install pnpm globally: npm install -g pnpm
• Docs: Tell users pnpm is a build requirement for Atom, or fall back on npm unstil pnpm is installed in the script/ dir in the next step.
• install pnpm in script/package[-lock].json.
• Make a getPnpmBinPath() function in script/config.js based on getNpmBinPath().
• Switch to pnpm in script/lib/install-script-dependencies.js with the new getPnpmBinPath() function from config.js
  • Make sure this works!
• Switch to pnpm in script/lib/install-apm.js just like above
  • Make sure this works!
• Well, that's it until/unless we get it working in apm.
  • Leave this until the above is verified twenty times over! Vet up front or you will spend many times as long debugging later.

[DeeDeeG]

Issue to resolve: pnpmdoesn't support the --global-style flag Atom uses to install apm.

(The flag changes how node_modules are installed, reducing deduping, and potentially changing which modules/version of modules are reachable from a given package).

Is this flag necessary in upstream? We should independently validate that dropping that flag is okay against upstream's master, as not otherwise modified for this fork (minimal tweaks to CI to allow CI to run are acceptable).

(Validating it with pnpm changes two variables at once, and isn't useful for debugging, troubleshooting or vetting purposes).

Status: in-progress. ~~Bug~~ Ask me about it if I don't post back.

Historical note: --global-style flag was added here: https://github.com/atom/atom/pull/11897, can infer this was to cope with dependency flattening circa npm@3, per the discussion in that PR. Could install an old npm@3 to see if it was needed even then or was just done preemptively, or out of an abundance of caution. :shrug: Currently validating whether this is not an issue with present-day code, i.e. npm@6 and [email protected].

[aminya]

Certainly, we should test this. But if we can make it work for atom, that is a good improvement!

I have started to use pnpm for other packages of this organization. It works without any issues. Check these for example: https://github.com/atom-ide-community/atom-ide-javascript https://github.com/atom-ide-community/atom-ide-datatip https://github.com/atom-ide-community/eslint-config-atomic

Regarding the lock file, I do not think it is a good idea to keep the original package-lock. If we decide to switch, we should do it completely. I always keep the lock update commits separate from the rest, so I can drop them in case of a rebase. If we do so, there will not be any issues.

With all of these being said, I do not want to consider this as our priority. Runtime behavior is more important for us, and we should invest in that mostly. For other packages of this organization, the switch is very easy and I did it instantly, but for something like Atom itself, I want to call this an experiment.

[ThatXliner]

• I think we should avoid using pnpm's pnpm-lock.yaml if pnpm will let us. (If it will let us just keep using package-lock.json as a lockfile, but still install faster in CI.) Remember, we maintain all diff from upstream ourselves. One less custom piece of different tech, one less bit of maintenance/bugfix/chore work to worry about. We don't want to spend time with this fork doing that kind of work, IMO.

Not a problem anymore 😄