-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: package-lock update #879
Conversation
Codecov Report
@@ Coverage Diff @@
## master #879 +/- ##
=======================================
Coverage 91.13% 91.13%
=======================================
Files 45 45
Lines 2053 2053
=======================================
Hits 1871 1871
Misses 182 182 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Audit issues are really annoying. Tests pass and 0 audit issues in lib now.
Thanks 👍
Edit: This test was flawed as I thought my NPM credentials changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked package.json again and found out all URLs point to a totalpave registry: https://registry.totalpave.com
@breautek Please rebuild package-lock.json again with the NPM registry setup.
Do we want to upgrade to the new lockfile version as well? My NPM started to print these warnings but I don't know the changes and consequences yet.
Will the older npm versions on older node versions on GHA properly handle the updated lockfile format? |
Argh! Oops! That's work stuff bleeding in... ill fix it when i get home |
Npm will still work with a message that "it will try its best". But not really sure what that means exactly. |
@NiklasMerz do you think there would be pushback if we start adding .npmrc configs to our cordova repos to re-assert npm registry is npms official registry? Would avoid mistakes like this in the future by having a project level npm config. |
f09df85
to
96309ed
Compare
Set my registry back to |
Mostly we should not be committing package-lock, except for the cli itself ... am I missing something? |
Package-lock is intended to be committed, as it ensures that two developers on two different machines will install the exact same dependencies when they run npm install. Not to be confused when users are using this package as a library, in which case their root package-lock is used. From NPM: https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json
|
This sums up what I think ... |
There was a consensus back in 2018 via apache/cordova#4 (comment) to add package-locks, which is why variety of our packages have package-locks. If you ask my personal opinion on package-locks, I hate them, mostly for the reasons described by sindresorhus. However, not committing them still presents the same issues described by sindresorhus, unless we (the maintainers) are constantly wiping the package-lock & node_modules and reinstalling from scratch. We could configure NPM via
Despite it's flaws I think the benefits of package-lock still outweighs the consequences of not committing/disabling package-lock. |
Okay, yeah that makes sense. |
Platforms affected
Motivation and Context
Resolves several sub-dependency vulnerabilities
Audit JSON Log
Description
Testing
Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)