Skip to content

build(deps): bump NPM from 8.5.1 to 8.18.0#5518

Merged
jeffwidman merged 2 commits intodependabot:mainfrom
THETCR:fix/locked-npm-version
Aug 23, 2022
Merged

build(deps): bump NPM from 8.5.1 to 8.18.0#5518
jeffwidman merged 2 commits intodependabot:mainfrom
THETCR:fix/locked-npm-version

Conversation

@THETCR
Copy link
Copy Markdown
Contributor

@THETCR THETCR commented Aug 11, 2022

Currently the NPM version being used is locked to version 8.5.1, which was released almost half a year ago.
Besides containing two security vulnerabilities, there are also issues with, for instance, overrides.

This PR makes NPM use a SemVer range and omits locking the minor and patch versions. Which prevents any sudden breaking changes (opposed to using @latest) while still keeping NPM up-to-date.

@THETCR THETCR requested a review from a team as a code owner August 11, 2022 21:07
Copy link
Copy Markdown
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Oof, being that outdated isn't great. 😢

However, we need to stick with a fully pinned version:

  • If we don't pin it we won't have a clear history of what npm version was in use with each release.
  • A new npm release could introduce a bug or performance regression which would get released as part of an unrelated change and be harder to detect.
  • If we did want to keep npm up to the latest a new release wouldn't necessarily pick it up because of docker caching.

That said, we certainly want to be up to date, so can you change this PR to bump the version to the latest stable release?

@THETCR THETCR force-pushed the fix/locked-npm-version branch from 6714392 to 9dec570 Compare August 18, 2022 18:44
@THETCR
Copy link
Copy Markdown
Contributor Author

THETCR commented Aug 18, 2022

@jeffwidman
Fair enough.
The only thing I'm a bit doubtful of is the Docker caching. Since NPM is pulled and installed from the npmjs registry by the NPM package that comes with NodeJS. Wouldn't that result in Docker not caching it?

I updated the PR for bumping NPM to 8.18.0

The reason I removed the preceding 'v' is because it's officially not a part of SemVer spec and technically invalid.
Although NPM tolerates it for convenience, because of the common use in the English language,

@THETCR THETCR changed the title fix: npm, only lock major version build(deps): bump NPM from 8.5.1 to 8.18.0 Aug 18, 2022
Copy link
Copy Markdown
Contributor

@pavera pavera left a comment

Choose a reason for hiding this comment

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

Thanks for updating this!

Copy link
Copy Markdown
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this!

@jeffwidman jeffwidman merged commit ab11442 into dependabot:main Aug 23, 2022
@pavera pavera mentioned this pull request Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants