package-lock.json changes after merging a Renovate PR and running npm install #2294
Comments
|
@felixfbecker Renovate depends on Maybe it has to do with the options passed to the Do you have a repository that can be pulled down to replicate the outdated dependency that Renovate is upgrading? Basically I would like to see if the options passed to It could also just be that's how |
|
Another related npm issue from the now-archived repo: npm/npm#17722 Here is an attempt to move that issue into the new npm.community: https://npm.community/t/package-lock-json-and-optional-packages/692 When I previously pinged the npm team about adding an option like |
|
I was browsing other npm issues and see that @felixfbecker has experience with npm’s optional dependency handling before: npm/npm#18202 (comment) :) |
|
I wanted to stress again that this is probably the number one friction point with Renovate and really takes the automation benefits away, as I always have to manually commit a new lockfile again. Since we also have some packages configured to automerge, I constantly get questions like "why does my lockfile always change? I tried reinstalling npm and clearing the cache but it still changes". Maybe we can express to npm that this is a really important issue for tools like Renovate, and if npm does not fix it soon, find a workaround in Renovate (e.g. updating lockfiles manually) |
|
This seems to be the latest bug tracking this in npm: https://npm.community/t/package-lock-json-keeps-changing-between-platforms-and-runs/1129 |
|
The recommendation from npm appears to be that your team should be running npm ci and not npm install. If this is their position then it explains somewhat why they don’t seem to be treating this with high priority. |
|
I don't understand what you/they mean by this - should I run |
|
I have pinged the npm team again just now to confirm, but I believe they intend for you and your team to run “npm ci” instead of “npm install” unless you are adding dependencies or modifying versions manually. I think you need to ignore the unintuitiveness of the command name and adopt their recommendation, assuming they confirm it. Meanwhile yes Renovate will run install because thats it’s job to update lock files. If it’s your job to just install packages from the lock file then you will run “npm ci”. I hope though the npm team will help solve the problem either by:
|
|
Can you confirm one thing: is Renovate adding the “optional” lines in your lockfile and removing them? And which platform are you and developers running “npm install” on when the file changes back? |
|
99% of our team runs on macOS, while I assume Renovate runs on Linux.
|
|
Thanks for the data points. I think the npm cli team assumes the performance of npm ci is fast enough for most/all situations but in your case it’s evidently adding an unnecessary 10s. Although it does wipe out node modules each time, npm ci should usually reinstall from cache so removing that directory is not actually as “inefficient” as it sounds. |
|
My understanding was that Some hard numbers: npm ci: 55.66snpm i (noop) 13.59s |
|
So if Renovate were to “work around” this, we’d need to do a diff/compare of the before and after lock files and strip out all newly-added “optional”: true lines. It’s not foolproof though because we’d need to strip it from newly-added dependencies too, and in theory here could be one that’s optional on both Linux and OSX and could get added again once npm install is run on OSX (i.e. edge case) |
|
I wonder if these packages are actually optional. If they truly are, then the Linux behaviour seems right (having an |
|
Yes, I think probably the Linux command is doing the right thing and the OSX one is problematic. Do you think a post install script could diff/grep/reset so that it doesn’t ultimately modify the lock file unnecessarily when run on OSX? I think you could probably “trust” that any optional: true line is meant to be there. |
|
It's not clear to me how a diff/reset would work in practice. The solution I had in my head was to walk the tree to check which dependencies are optional or not. But that would probably require reverse-engineering npm's algorithm to a point where we could just fix the bug in npm itself. |
|
I seem to be running into a related issue. I recently changed our repo to use In addition, it also seems to add These changes to the I receive the same error when checking out the PR locally and running It seems this may be related to this npm issue. We are using If I check out the Renovate PR locally and run I pushed the updated Any idea what could be going on and how this could be fixed? |
|
@martijnwalraven I suspect that the By the way you are welcome to leave dependencies in ranges and configure Renovate to leave them alone, although usually I'd ask people "why?" first, especially for |
|
@martijnwalraven I think there might be more going on - let's move yours to a separate issue. When I check out that repo and run
|
|
Confirmed that @martijnwalraven's problem is another |
|
@felixfbecker I was checking back through this issue and I don't think that we have any steps or example repository to reproduce this in yet? |
|
@rarkins Thanks very much for taking a look at the problem @martijnwalraven (who is out on holiday at the moment) identified above — and for moving it to a new issue, and for reporting the (ultimate) cause of the bug to npm.community. ⭐️🙇⭐️ |
|
Here is a second npm.community post on the topic: https://npm.community/t/package-lock-json-changes-from-one-npm-install-to-the-next/1454/5 |
|
I've also run into the same issue, would it be possible to tell renovate which nodejs/npm version to use when bumping packages rather than trying to work around each specific case like those |
|
This happens even if the versions match exactly. It's the OS differences. |
|
Yes, but it's my understanding that not every npm version behaves that way. |
|
I'm hoping to support configurable |
|
It means you won’t get any package lock updates at all so probably not a good option. It’s disappointing that this hasn’t been prioritised by npm yet. For anyone in this scenario, lockfiles are essentially “broken” |
|
By the way this one is difficult for Renovate to work around because it’s really the OSX client that’s misbehaving here and stripping out optional statements it shouldn’t. Ie not the Linux client that Renovate uses, which is correctly adding optional lines where they should be. But faced with no fix, I’m considering trying. I think this logic might cover the majority of cases:
There’s surely going to be some cases when it causes some other problem, but if it’s made opt-in (config-wise) then could be worth trying. |
|
I feel like that fix might just flip the problem on its head by removing I also imagine it might make troubleshooting the npm issue more problematic if we're modifying the lockfile programatically. Perhaps a config option just adds complexity to the problem without guaranteeing to solve it? I think it's worth some investigation though. |
|
For the record, we switched to yarn because of this and broken npm link. |
|
@felixfbecker P.S. I ❤️ npm though |
|
Discussion re: this particular |
|
This fix in npm should resolve our |
|
This fix has been released in npm 6.6.0. @rarkins will latest npm be available to renovate immediately? |
|
@srilq it will be merged in #3094 soon |
|
@rarkins Yay, thanks |
|
@srilq I don't see it mentioned in the changelog: https://github.com/npm/cli/releases/tag/v6.6.0 Could you link to the fix? |
|
@felixfbecker |
|
It looks like this fix hasn't changed the Linux release that renovate uses, but instead changes how the macOS release works to be in line with the Linux version (which makes sense, see this comment: #2294 (comment)). I think this issue can be resolved, but end-users need to make sure they are running npm>=6.6.
Update: this npm registry issue is fixed! (Source: https://npm.community/t/some-packages-have-dist-tarball-as-http-and-not-https/285/50). If you see the lockfile problem locally, you just need to run this and it should work as a permanent fix: |
|
That sounds like the right fix to me. The problem wasn't adding the If you see Renovate adding in |
|
After updating locally to npm 6.7 the issue with --- a/package-lock.json
+++ b/package-lock.json
@@ -3878,28 +3878,28 @@
"dependencies": {
"abbrev": {
"version": "1.1.1",
- "resolved": "https://registry.npmjs.org/abbrev/-/abbrev-1.1.1.tgz",
+ "resolved": false,
"integrity": "sha512-nne9/IiQ/hzIhY6pdDnbBtz7DjPTKrY00P/zvPSm5pOFkl6xuGrGnXn/VtTNNfNtAfZ9/1RtehkszU9qcTii0Q==",
"dev": true,
"optional": true
},
"ansi-regex": {
"version": "2.1.1",
- "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-2.1.1.tgz",
+ "resolved": false,
"integrity": "sha1-w7M6te42DYbg5ijwRorn7yfWVN8=",
"dev": true,
"optional": true
},
"aproba": {
"version": "1.2.0",
- "resolved": "https://registry.npmjs.org/aproba/-/aproba-1.2.0.tgz",
+ "resolved": false,
"integrity": "sha512-Y9J6ZjXtoYh8RnXVCMOU/ttDmk1aBjunq9vO0ta5x85WDQiQfUF9sIPBITdbiiIVcBo03Hi3jMxigBtsddlXRw==",
"dev": true,
"optional": true
},
"are-we-there-yet": {
"version": "1.1.5",
- "resolved": "https://registry.npmjs.org/are-we-there-yet/-/are-we-there-yet-1.1.5.tgz",
+ "resolved": false,
"integrity": "sha512-5hYdAkZlcG8tOLujVDTgCT+uPX0VnpAH28gWsLfzpXYm7wP6mp5Q/gYyR7YQ0cKVJcXJnl3j2kpBan13PtQf6w==",
"dev": true,
"optional": true,The |
|
@westonruter could you copy paste this to a new issue? Btw Renovate is running npm 6.9.0 but soon switching to a mode where it will try to detect which version the project is using, and also allow config override of it too. |
What Renovate type are you using?
Renovate GitHub App
Describe the bug
After merging a Renovate PR and running
npm installlocally, package-lock.json changes.This is unfortunate, because it means that the automation still requires a manual
npm install, commit and push, at which point I could have just updated the dependency manually in the first place.I am running npm v6.2.0 (latest). What version is Renovate using?
Diff
The text was updated successfully, but these errors were encountered: