Fix: Lockfile should be updated when merge conflcits are resolved #4195
Conversation
**Summary** Follow up to #3544. Currently, `yarn` happily keeps moving if it detects merge conflicts and is able to resolve them in the lockfile. That said it doesn't persist the resolution by saving the lockfile to disk again. This patch ensures writing the lockfile if it is "dirty". This patch also causes `yarn` to throw an error if there are merge conflicts in the file and `--frozen-lockfile` option is true. **Test plan** Existing unit tests. Also try running `yarn install` with the following files: `package.json` ``` { "name": "yarnlock-auto-merge", "version": "1.0.0", "main": "index.js", "author": "Burak Yigit Kaya <[email protected]>", "license": "MIT", "dependencies": { "left-pad": "^1.1.3", "right-pad": "^1.0.1" } } ``` `yarn.lock` ``` <<<<<<< HEAD left-pad@^1.1.3: version "1.1.3" resolved "https://registry.yarnpkg.com/left-pad/-/left-pad-1.1.3.tgz#612f61c033f3a9e08e939f1caebeea41b6f3199a" ======= right-pad@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/right-pad/-/right-pad-1.0.1.tgz#8ca08c2cbb5b55e74dafa96bf7fd1a27d568c8d0" >>>>>>> right-pad ``` Without the patch, `yarn` won't update the lockfile. With the patch, the lockfile is replaced with the merged version.
__tests__/commands/_helpers.js
Outdated
| @@ -38,7 +38,7 @@ export async function createLockfile(dir: string): Promise<Lockfile> { | |||
| lockfile = parse(rawLockfile).object; | |||
| } | |||
|
|
|||
| return new Lockfile(lockfile); | |||
| return new Lockfile(lockfile, undefined, 'success'); | |||
There was a problem hiding this comment.
Shouldn't the third parameter be an optional object that takes a parseResult key?
There was a problem hiding this comment.
The third parameter is just the parse result type, not the parse result itself. May be I should rename the variable to make that clear?
There was a problem hiding this comment.
Oh yeah I see, yep, probably. But my point was more about avoiding unnamed arguments (new Lockfile(..., { parseResultType: 'success' })).
src/lockfile/wrapper.js
Outdated
| } else { | ||
| if (reporter) { | ||
| reporter.info(reporter.lang('noLockfileFound')); | ||
| } | ||
| } | ||
|
|
||
| return new Lockfile(lockfile, rawLockfile); | ||
| return new Lockfile(lockfile, rawLockfile, parseResult && parseResult.type); |
There was a problem hiding this comment.
I think a ternary would better convey the semantic, wdyt? The && behavior kinda obfuscates the fact that the return value is parseResult.type rather than a boolean.
There was a problem hiding this comment.
x && x.y is a pattern that I see used in many places to protect against "potentially null x" situations but if you strongly think that a ternary would be better, I'll change it.
|
@arcanis updated to use named arguments. Looks much better, thanks! |
|
So does this mean that the functionality of doing |
|
The functionality is working since yarn resolves the conflict in memory and happily moves on. Also in most cases, node_modules is not up to date which triggers rewriting the lock file. This applies to situations where there's a lockfile with conflicts but node_modules is somehow up to date, already reflecting the resolved state. This patch just makes sure the file is written if merge conflicts are detected. It also causes an error in frozen lockfile mode though since the lockfile needs an update. |
Summary
Follow up to #3544. Currently,
yarnhappily keeps moving if itdetects merge conflicts and is able to resolve them in the lockfile.
That said it doesn't persist the resolution by saving the lockfile
to disk again. This patch ensures writing the lockfile if it is
"dirty".
This patch also causes
yarnto throw an error if there are mergeconflicts in the file and
--frozen-lockfileoption is true.Test plan
Existing unit tests. Also try running
yarn installwith the followingfiles:
package.jsonyarn.lockWithout the patch,
yarnwon't update the lockfile. With the patch,the lockfile is replaced with the merged version.