This repository has been archived by the owner. It is now read-only.

Auto-detect and merge lockfile conflicts #18007

Closed
gerges opened this Issue Jul 30, 2017 · 14 comments

Comments

Projects
None yet
9 participants
@gerges

gerges commented Jul 30, 2017

What's the feature?

package-lock.json conflicts are difficult to manually resolve and regenerating the package-lock opts into often unwanted upgrades of dependencies and their children. npm should be able to detect conflicts in the package-lock.json and correct them.

What problem is the feature intended to solve?

Large teams can very often run into merge conflicts of package-lock.json when dependencies are added, upgraded, or removed in branches. These conflicts can be incredibly difficult to resolve manually, the often suggested solution is to remove and re-generate package-lock.json, this often opts you into unwanted upgrades of packages and children (npm5 is now deterministic, but not idempotent from only a package.json`)

Is the absence of this feature blocking you or your team? If so, how?

Currently my team manually merges the package-lock.json and attempts to re-run npm i and add any modifications to package-lock.json to the merge/rebase. This is prone to error.

Is this feature similar to an existing feature in another tool?

Yes. It was recently added in yarn. yarnpkg/yarn#3544

Is this a feature you're prepared to implement, with support from the npm CLI team?

Assist in, but I don't believe I'm best suited to implement.

@travi

This comment has been minimized.

Contributor

travi commented Oct 3, 2017

possibly another way to look at this could be providing a way to force regeneration of the lockfile.

since the lockfile should really only ever be modified by npm, i would rather treat the lockfile as binary in git and regenerate it from the package.json if there is ever a conflict. the issue with this is that git does not provide any markers in conflicting binary files, so trying to auto-resolve conflicts would possibly be more difficult than simply having the ability to start over.

this option might not handle all use cases, but it would cover mine pretty well and allow marking the lockfile as binary in git.

@wzrdtales

This comment has been minimized.

wzrdtales commented Oct 29, 2017

Can just enter in here, the package-lock.json just like yarn.lock are banned from our projects, as they create more problems than they actually solve.

As these problems actually only exits b/c with the new autogenerated lockfiles in place, they do break any and every workflow. and merge requests in general, this feature is somehow necessary. You only need to have two developers making in two merge requests two different changes to the package.json which are legitimate and entering their new version of package-lock.json. At this point the best option would be to actually throw the package-lock.json away rather than really wasting time on merging these changes together.

To solve this either a proper way to merge them easily is needed or to question and revert the per default generation of this file on every npm install, just like it was before. The reason for this is simply that it feels like package-lock.json is rather a replacement for package.json than an extension. If I would wanted to have my packages locked down I could always have specified a locked in version instead of a selector. But now these selectors actually mutated from automatically getting the latest version depending on these versions to being a helper when explicitly upgrading packages, which destroys somehow the original purpose. Instead I would want to lock down versions by intention and not by accident and really only if I go to production with them.

So maybe going this way per default is just the wrong way, but maybe my opinion is just plainly wrong. I'm eager to hear your opinion and your explanation to that.

@jacob-hd

This comment has been minimized.

jacob-hd commented Nov 2, 2017

@travi Regenerating the lockfile from the package.json defeats the purpose of having a lockfile in the first place, as it will result in upgraded dependencies being brought in, as @gerges mentioned several times in his write-up above. The purpose of the lockfile is to freeze your node_modules folder and ensure that all deployments of a given project will have identical code in that folder.

@travi

This comment has been minimized.

Contributor

travi commented Nov 3, 2017

@jacob-hd as i mentioned, regenerating doesnt handle all use cases, but could solve some valuable ones if it were simpler. however, it doesnt necessarily defeat the purpose of having a lockfile. i specify exact versions in my package.jsons, so the lockfile is mostly beneficial for locking second level and deeper dependencies.

the biggest risk that the lockfile solves for me is those non-direct dependencies releasing an update that is in-range of my direct dependency that is breaking, which gets deployed without any commit happening. i'm far less worried about those versions updating as a result of regenerating a lockfile in a way that results in a commit that has a chance to be verified before moving forward.

@darkobits

This comment has been minimized.

darkobits commented Nov 7, 2017

Merge conflicts appear to be an inevitable, and largely unforeseen consequence of working with lockfiles. Yarn seems to have been quick to address this issue, and NPM needs to do the same.

In fact, I would argue that it is more important for NPM to address this issue because its lockfiles are significantly larger -- and therefore significantly more unwieldy -- than Yarn's.

Regenerating the lockfile is not a viable long-term solution because it is destructive. I would rather refer to this option as "resetting the lockfile", as regenerating connotes that a lockfile is somehow a deterministic function of the contents of package.json, which it is not. It is a single source of truth that, when deleted, is lost forever.

As someone who is trying to convince a team of engineers to begin using lockfiles, this is a pain-point that is hindering adoption and will make compliance an issue going forward.


TL;DR Without automatic merge conflict resolution, the negative impact on developer experience makes working with lockfiles an unreasonable ask for teams.

@travi

This comment has been minimized.

Contributor

travi commented Nov 9, 2017

I like the distinction you made between conflict resolution and resetting the lockfile. I do think there are cases where resetting would be useful and have been frustrated by the lack of being able to do so easily. however, you are completely correct that it is a separate need from repairing a lockfile that you want to maintain w/o resetting.

jnnrdn added a commit to raket/wally-theme that referenced this issue Dec 7, 2017

@crussell52

This comment has been minimized.

crussell52 commented Dec 19, 2017

Just recently ran into this... FWIW, this is how we've started dealing with it...

We generally have a "more stable" (i.e. master) and a "less stable" (i.e. topic) branch. When we run into a conflict, we accept the "more stable" version of the file and re-run npm install in the "less stable" branch.

It is possible for the topic branch to receive an incidental update, but the fact that they are fairly short lived limits exposure to that. The versions of the more stable branch are respected, which seems like the more important part.

Still not perfect because it has to be manually done each time a lock-file change goes into the more stable branch and I'd love to see something added to npm to assist with this challenge.

@jacob-hd

This comment has been minimized.

jacob-hd commented Dec 19, 2017

@crussell52 You seem to be implying that you're editing a lock file by hand, or letting git merge the lock files (which constitutes an edit). However, lock files should never be edited by anything other than the npm process.

@russelltrafford

This comment has been minimized.

russelltrafford commented Dec 19, 2017

I'm thinking the easiest and semi-decent way to handle this is to use a .gitattributes file and mark the lock file as a binary. That way it should just choose a version rather than try to merge the internals right?

But I agree, npm should address this properly.

@travi

This comment has been minimized.

Contributor

travi commented Dec 19, 2017

@jacob-hd while i agree with you that only the npm process should touch this file, it does not appear that the npm team agrees.

@russelltrafford i mentioned above that i think it should be marked as binary, but that has proven to be more of a pain than it is worth. this ends up with the file having a conflict any time it changes from two different sources. while i agree that this is at least somewhat desired, it results in almost every PR that includes a package change needing to have conflicts resolved if not pulled in immediately.

i've decided to stop marking as binary and let git merge as best as it can unless the npm team can find a way to enable truly letting the npm cli be the only thing that modifies the lockfile

@crussell52

This comment has been minimized.

crussell52 commented Dec 19, 2017

@jacob-hd I agree that a built-in handling is the way to go here. I'm just sharing my current process to (maybe) help others and maybe spark some ideas within the npm team.... or maybe I'll find out that what I'm doing is a terrible idea... I am, after all, relatively new to the node world. 😄

To clarify, I am not making any manual entries into the lock file -- instead, I'm basically abandoning changes made in the topic branch by completely accepting the version coming in from master and then having npm install re-create the lock entries for the topic branch.

This can absolutely introduce new risk into the topic branch because new versions of dependencies can be brought in, so tests must be re-ran, and such. But at least the already-approved and merged dependency versions (those in master) are respected.

@boxfoot

This comment has been minimized.

boxfoot commented Dec 19, 2017

We've been using an approach like @crussell52 's. A partial solution could be a build script (or even small npm module) that does the following:

  1. git checkout --theirs package-lock.json
  2. Parse and compare dependencies listed in both versions of package.json
  3. Re-run npm install and npm remove on the changed dependencies so that package-lock.json reflects the dependencies listed in the topic's package.json
@gerges

This comment has been minimized.

gerges commented Feb 22, 2018

It appears this is fixed with e27674c

@MrSnappingTurtle

This comment has been minimized.

MrSnappingTurtle commented Jun 29, 2018

From the commit mentioned above,

Resolving lockfile conflicts

Occasionally, two separate npm install will create package locks that cause
merge conflicts in source control systems. As of npm@5.7.0, these conflicts
can be resolved by manually fixing any package.json conflicts, and then
running npm install [--package-lock-only] again. npm will automatically
resolve any conflicts for you and write a merged package lock that includes all
the dependencies from both branches in a reasonable tree. If
--package-lock-only is provided, it will do this without also modifying your
local node_modules/.

To make this process seamless on git, consider installing
npm-merge-driver, which will teach git how
to do this itself without any user interaction. In short: $ npx npm-merge-driver install -g will let you do this, and even works with
pre-npm@5.7.0 versions of npm 5, albeit a bit more noisily. Note that if
package.json itself conflicts, you will have to resolve that by hand and run
npm install manually, even with the merge driver.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.