Remove dev-dependency on npm-check-updates.#959
Merged
Conversation
It appears that the intention of including `npm-check-updates` in this repository was to provide an easy way to update dependent packages from the console. This was originally introduced in [Ref 0]. Unfortunately, the `npm-check-updates` package has an explicit dependency on `npm@3`, which means that it includes the entire `npm` in its `node_modules`. I originally observed this when analyzing the test failures in [Ref 1] and [Ref 2] which indicated (in its npm error) that the npm version was v3.10.10. The explanation for this is relatively straight-forward: Older versions of npm uses an nested tree of `node_modules` directories while newer versions hoist `node_modules` (i.e. flatten) to the highest location possible and only nesting dependencies to meet conflicting version constraints. Unfortunately, that means that once `npm@3` was installed by the newer `npm@5` as a dependency of `npm-check-updates`, it was hoisted to the top-level `node_modules` directory, exposing `npm@3` to `npm-scripts` which directly call `npm` commands on the nested packages, as is done on this monorepo (with, for example the `postinstall` script which runs `npm run compile` in each of the server middleware packages). :face_palm: Overall, this package doesn't seem to provide enough value for us to continue to include it, especially since we have Renovate - coupled with the same information being available from `npm outdated` and `npm update` commands. I'm not sure why we would want to install newer versions than our "package.json" allows. Futhermore, looking at the issue tracker on `npm-check-updates`, I'm assuming this is the reason I've seen "`dezalgo`" install errors [Ref 3] on this repository recently and _possibly_ even a bizarrely corrupted npm cache I encountered recently (though admittedly, that could be a stretch). See also: [Ref 4]. [Ref 0]: d7ca07f7 [Ref 1]: https://circleci.com/gh/apollographql/apollo-server/1915 [Ref 2]: https://app.netlify.com/sites/apollo-server-docs/deploys/5acdd3b7 [Ref 3]: raineorshine/npm-check-updates#420 [Ref 4]: raineorshine/npm-check-updates#423
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
It appears that the intention of including
npm-check-updatesin this repository was to provide an easy way to update dependent packages from the console. This was originally introduced in Ref 0.Unfortunately, the
npm-check-updatespackage has an explicit dependency onnpm@3, which means that it includes the entirenpmin itsnode_modules.I originally observed this when analyzing the test failures in Ref 1 and Ref 2 which indicated (in its npm error) that the npm version was v3.10.10.
The explanation for this is relatively straight-forward: Older versions of npm uses an nested tree of
node_modulesdirectories while newer versions hoistnode_modules(i.e. flatten) to the highest location possible and only nesting dependencies to meet conflicting version constraints.Unfortunately, that means that once
npm@3was installed by the newernpm@5as a dependency ofnpm-check-updates, it was hoisted to the top-levelnode_modulesdirectory, exposingnpm@3tonpm-scriptswhich directly callnpmcommands on the nested packages, as is done on this monorepo (with, for example thepostinstallscript which runsnpm run compilein each of the server middleware packages). :face_palm:Overall, this package doesn't seem to provide enough value for us to continue to include it, especially since we have Renovate - coupled with the same information being available from
npm outdatedandnpm updatecommands. I'm not sure why we would want to install newer versions than our "package.json" allows.Futhermore, looking at the issue tracker on
npm-check-updates, I'm assuming this is the reason I've seen "dezalgo" install errors (Ref 3) on this repository recently and possibly even a bizarrely corrupted npm cache I encountered recently (though admittedly, that could be a stretch).See also: Ref 4.