Respect the "NO_APM_DEDUPE" env var on Windows (affects the postinstall script)

[DeeDeeG]

Requirements

• Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
• All new code requires tests to ensure against regressions

Description of the Change

Linux and macOS have respected a NO_APM_DEDUPE env var for a long time. If this env var is set, the postinstall script skips deduping (for these OSes, this is handled in script/postinstall.sh).

This PR adds a couple of lines to the Windows-specific script\postinstall.cmd file to do the same thing on Windows.

Alternate Designs

None.

Benefits

This should enable installing apm with npm ci --global-style in Atom's script/bootstrap.

Which in turn means we can have reliable, stable bootstrapping, when desired, for the Atom repo.

That turned out not to be true. So, there is very little benefit. At least an env var that works on Linux and macOS will work on Windows with this PR merged. But there is little difference when installing apm with or without deduping.

Update: This does work, in fact, in the usual use-case. Just not when specifying apm as a git URL dependency.

Possible Drawbacks

None.

Verification Process

For this repository on its own:

I ran npm run postinstall and npm install with this env var set (set NO_APM_DEDUPE=true in cmd.exe).

Deduping was successfully skipped, and a message was printed out (>> Deduplication disabled) to indicate that this is happening on purpose.

---

I also tested installing apm in Atom with npm ci like this:

• cd atom\apm
• Set NO_APM_DEDUPE=true
• npm ci
• During npm ci, quickly after apm is extracted, copy over the modified script\postinstall.cmd and overwrite the existing, non-patched one. We want to patch script\postinstall.cmd in a stable version of apm.
  • If you are not on Windows, then there is no need to copy or patch any files. The flag is already respected on Linux/macOS.
• Observe that the following is printed at the end: >> Deduplication disabled
• Run npm ls, and observe that no packages are reported missing.

(Live-patching this file will no-longer be needed once this PR is merged and shipped in a stable release of apm.)

Applicable Issues

None.

[DeeDeeG]

Hmm...

This should enable installing apm with npm ci --global-style in Atom's script/bootstrap.

Which in turn means we can have reliable, stable bootstrapping, when desired, for the Atom repo.

I am disappointed to find that this was not actually true. It doesn't matter if deduping is disabled or not. npm ci simply doesn't install all the dependencies if using the --global-style flag. So this PR is mostly pointless.

It still allows disabling deduping with the same env var as on Linux/macOS, but there is little difference when installing apm with or without deduping.

[DeeDeeG]

Okay, I found out this does work in normal circumstances! It just doesn't work very well with using npm ci to install apm as a git URL dependency. (But we already avoid doing that anyway, because of how weird it acts).

Testing notes (click to expand):

It does work some of the time. Investigating whether this is:

• A problem with npm ci only when installing apm as a git URL dependency
  • Seems likely. Worked for me when patching in-place a stable apm install from the npm registry.
• a Windows-specific problem
  • Maybe? Worked for me on macOS.
  • Okay, this is working for me on Windows when patching apm\postinstall.cmd in-place in stable apm. Not working when trying to install apm as a git URL dependency.
• Perhaps npm ci and the --global-style flag don't work together?
  • Nope. This is working with npm ci and npm ci --global-style. Tested on macOS and Windows.

@aminya if you want to enable installing apm with npm ci (stable dependencies, no unexpected build failures!) then please consider merging this PR.

---

... I will make a PR for upstream apm as well. But merging here allows for a quick fix for building the Atom fork. Just need to do a follow-up PR over at the Atom fork to wire up script/bootstrap --ci to install apm with npm ci and the NO_APM_DEDUPE env var set. And then upgrade to this updated version of apm, probably manually downgrading git-utils to the non-broken version in package-lock.json to make it build-able.

[DeeDeeG]

Here's the upstream PR: atom#912

[aminya]

Can't we get rid of deduping altogether? Does deduping make any difference in the size of node_modules?

The impression I get from this blog post is that deduping happens automatically. Running it afterward does not have any effect.
https://blog.npmjs.org/post/618653678433435649/npm-v7-series-arborist-deep-dive

[DeeDeeG]

Yes we can eliminate deduping for this fork.

I was thinking about this, and even for upstream I am considering proposing that the default change to "not deduping."

The impression I get from this blog post is that deduping happens automatically

Yes, I think that's mostly true.

I think npm is slightly less eager to dedupe when installing over an existing node_modules folder??? I know that usually with no lockfile and no node_modules folder, npm install maximally dedupes the dependency tree.

There are some cases where npm dedupe will lighten up the node_modules by a few dependencies. And these bytes are somewhat precious, as apm's node_modules are included 100% in the Atom bundle.

[Edit: actually, the same technique is used as with the rest of Atom's dependencies... If I read correctly, the build scripts require and load the modules to identify whatever files are needed, and then the scripts selectively include only necessary files in Atom's bundled apm. But still, eliminating some duplicated dependencies == a smaller bundle. Making/using an optimized dependency tree is still good to do.]

But most of the time, running npm dedupe is not needed. And it's a somewhat dangerous command, given that it does not always leave you with all your needed dependencies. I feel npm dedupe isn't as well-tested as npm install. It frequently needs following up with npm install again to fetch the missing packages...

One last thought: This only matters when upgrading apm bundled in Atom. All other times we should expect to install what's in the lockfile. (But upgrading updates the lockfile, so we should be careful to dedupe then.)

In summary, my thoughts:

I think this should be off by default, but people should still be in the habit of manually running npm dedupe when they bundle a new version of apm with Atom.

[DeeDeeG]

@aminya which option do you like best for this fork?:

• Controllable by a flag, but "off by default"
• Controllable by a flag, but "on by default"
• Deleted; Never dedupe during the postinstall script, ever.

(Whatever we pick, I want to handle it the same way in both the postinstall.sh for Linux/macOS and the postinstall.cmd for Windows.)

[aminya]

Suggested change

	setx JOBS 16
	setx JOBS max

[DeeDeeG]

Steps left to fix the build for community fork of Atom:

• Get this version of apm (or similar/with deduping completely removed) updated in atom-community/atom
  • May need to manually downgrade git-utils to the version that still builds (v5.6.2), by editing package-lock.json??
• Set up script/lib/install-apm.js so it sets NO_APM_DEDUPE to some greater-than-zero-length value, if need be.
  • Probably want to enable npm ci for the --ci flag in scipt/lib/install-apm.js

If you want me to open PRs for any of this, I'm up for it.

[aminya]

Let's remove deduping now.