Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

script: Remove automatic deduping from postinstall #924

Merged
merged 1 commit into from
May 20, 2021

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented May 19, 2021

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

This PR removes a couple of lines in script/postinstall.cmd and script/postinstall.sh that performed an npm dedupe toward the end of the postinstall script when installing apm itself.

Alternate Designs

  • Could keep automatic deduping in, but make it off by default/enabled with an env var
    • (I wonder if anyone would use the env var and enable deduping? Is that any easier than just running npm dedupe manually?)
    • Example implementation: 381120d..a93c88a
  • Could keep the status quo where it is on by default but can be disabled with an env var
    • (it seems to me that automatic deduping can cause problems and should not be on by default.)

I think off by default is justified, but removing the capability altogether is even cleaner to simplify the code. If a user wants to ensure apm is installed with maximum deduping, they can delete apm's node_modules folder and its package-lock.json, and reinstall.

In my experience, the extra npm dedupe run makes no difference.

Benefits

Faster installs of apm itself (by skipping deduping). No more missing dependencies when installing apm with npm ci; Makes installing apm with npm ci possible without having to set the NO_APM_DEDUPE env var anymore.

Possible Drawbacks

(Very small?) chance of missing some deduping/filesize savings when upgrading apm in the Atom repo.

To give modern npm the easiest time with its own automated/internal deduping when updating apm in the Atom repo, I recommend deleting apm's node_modules and package-lock.json, then updating its package.json to the newest apm, then running npm install --global-style as usual.

Verification Process

As intended with this change, npm dedupe is no-longer run during the postinstall script. I have tried this many times locally, and this has been in the community fork for several months now, including in CI.

Applicable Issues

Upstreaming this PR from the atom-community fork of apm: atom-community#71

As requested by @sadick254 in atom/atom#22450 (review).

Deduping leaves you with missing packages when installing apm with
`npm ci`. Modern npm should dedupe automatically to some extent,
so running dedupe isn't really useful, something like 99% of the time.

If a user wants to ensure apm is installed with maximum deduping,
they can delete apm's `node_modules` and `package-lock.json` and then
re-install.

(cherry picked from commit e3218fb,
which was merged previously at the atom-community fork of apm.)
@DeeDeeG DeeDeeG force-pushed the no-deduping-in-postinstall branch from 29e2ecf to 7be56b8 Compare May 19, 2021 21:42
@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented May 19, 2021

Force pushed to update the commit message to be more accurate. And I also edited the PR body text.

The rather contrived process to manually dedupe would be:

  • install apm as usual, with npm install --global-style atom-package-manager
  • Dedupe with the following commands: cd node_modules/atom-package-manager and npm dedupe
  • Return to the outer folder cd ../../
  • Make sure there are no missing dependencies with npm ls, and if there are any missing dependencies, restore them with npm install --global-style --ignore-scripts
  • Run npm ls again to ensure no dependencies are missing
  • Run git diff package-lock.json (or git diff --stat package-lock.json) to check if anything actually changed in package-lock.json. (Chances are high that nothing actually changed, and that this manual deduping was pretty much pointless.)
  • If anything actually changed -- if anything was successfully deduped without resulting in missing dependencies -- you can git add and git commit the updated package-lock.json

In my experience that effectively does nothing. npm already produces the optimized tree. So I edited the commit message and PR body to stop recommending that as a manual alternative. To reiterate: I do not think it is needed at all.

If someone wants to manually do something to reassure themselves that apm is installed with maximum deduping, I think it's much simpler to just delete apm's node_modules and package-lock.json and reinstall, so modern npm can create its internal "ideal tree" which is already deduped for you, internally to npm. apm's postinstall deduping has been redundant to that for some time now.

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented May 19, 2021

To clarify my edits:

The only situation in which I think npm install might fail to dedupe some things optimally, is if it is reinstalling over an existing node_modules folder filled with dependencies, and with an existing package-lock.json. It might use the existing dependencies from those sources as a a shortcut, or to try to make a narrower/more-limited change to the dependency tree.

That's why I think the best way to ensure apm is maximally deduped is to delete node_modules and package-lock.json and reinstall.

Given how easy that is, I don't think manually running npm dedupe is worth doing or worth recommending.

Copy link
Contributor

@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. 👍 Again your description on this PR is on point 🚀

@sadick254 sadick254 merged commit 0057189 into atom:master May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants