Fix: Lockfile should be updated when merge conflcits are resolved (#4195

)

**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 <byk@fb.com>",
  "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

@@ -38,7 +38,7 @@ export async function createLockfile(dir: string): Promise<Lockfile> {
     lockfile = parse(rawLockfile).object;
   }
 
-  return new Lockfile(lockfile);
+  return new Lockfile({cache: lockfile, parseResultType: 'success'});
 }
 
 export function explodeLockfile(lockfile: string): Array<string> {

__tests__/lockfile.js

@@ -46,16 +46,20 @@ test('Lockfile.fromDirectory', () => {});
 
 test('Lockfile.getLocked', () => {
   const lockfile = new Lockfile({
-    foo: 'bar',
-    bar: {},
+    cache: {
+      foo: 'bar',
+      bar: {},
+    },
   });
   expect(!!lockfile.getLocked('foo')).toBeTruthy();
 });
 
 test('Lockfile.getLocked pointer', () => {
   const lockfile = new Lockfile({
-    foo: 'bar',
-    bar: {},
+    cache: {
+      foo: 'bar',
+      bar: {},
+    },
   });
   expect(!!lockfile.getLocked('foo')).toBeTruthy();
 });
@@ -66,8 +70,10 @@ test('Lockfile.getLocked no cache', () => {
 
 test('Lockfile.getLocked defaults', () => {
   const pattern = new Lockfile({
-    foobar: {
-      version: '0.0.0',
+    cache: {
+      foobar: {
+        version: '0.0.0',
+      },
     },
   }).getLocked('foobar');
   expect(pattern.registry).toBe('npm');
@@ -76,7 +82,7 @@ test('Lockfile.getLocked defaults', () => {
 });
 
 test('Lockfile.getLocked unknown', () => {
-  new Lockfile({}).getLocked('foobar');
+  new Lockfile({cache: {}}).getLocked('foobar');
 });
 
 test('Lockfile.getLockfile', () => {

__tests__/package-request.js

@@ -8,7 +8,7 @@ import Config from '../src/config.js';
 
 async function prepareRequest(pattern, version, resolved): Object {
   const privateDepCache = {[pattern]: {version, resolved}};
-  const lockfile = new Lockfile(privateDepCache);
+  const lockfile = new Lockfile({cache: privateDepCache});
   const reporter = new reporters.NoopReporter({});
   const depRequestPattern = {
     pattern,

src/cli/commands/import.js

@@ -302,6 +302,6 @@ export function hasWrapper(commander: Object, args: Array<string>): boolean {
 }
 
 export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
-  const imp = new Import(flags, config, reporter, new Lockfile({}));
+  const imp = new Import(flags, config, reporter, new Lockfile({cache: {}}));
   await imp.init();
 }

src/cli/commands/install.js

@@ -347,14 +347,15 @@ export class Install {
     if (!lockfileCache) {
       return false;
     }
+    const lockfileClean = this.lockfile.parseResultType === 'success';
     const match = await this.integrityChecker.check(patterns, lockfileCache, this.flags, workspaceLayout);
-    if (this.flags.frozenLockfile && match.missingPatterns.length > 0) {
+    if (this.flags.frozenLockfile && (!lockfileClean || match.missingPatterns.length > 0)) {
       throw new MessageError(this.reporter.lang('frozenLockfileError'));
     }
 
     const haveLockfile = await fs.exists(path.join(this.config.lockfileFolder, constants.LOCKFILE_FILENAME));
 
-    if (match.integrityMatches && haveLockfile) {
+    if (match.integrityMatches && haveLockfile && lockfileClean) {
       this.reporter.success(this.reporter.lang('upToDate'));
       return true;
     }
@@ -708,13 +709,15 @@ export class Install {
       const manifest = this.lockfile.getLocked(pattern);
       return manifest && manifest.resolved === lockfileBasedOnResolver[pattern].resolved;
     });
+
     // remove command is followed by install with force, lockfile will be rewritten in any case then
     if (
+      !this.flags.force &&
+      this.lockfile.parseResultType === 'success' &&
       lockFileHasAllPatterns &&
       lockfilePatternsMatch &&
       resolverPatternsAreSameAsInLockfile &&
-      patterns.length &&
-      !this.flags.force
+      patterns.length
     ) {
       return;
     }

src/lockfile/parse.js

@@ -15,8 +15,10 @@ type Token = {
   value: boolean | number | string | void,
 };
 
-type ParseResult = {
-  type: 'merge' | 'none' | 'conflict',
+export type ParseResultType = 'merge' | 'success' | 'conflict';
+
+export type ParseResult = {
+  type: ParseResultType,
   object: Object,
 };
 
@@ -401,5 +403,5 @@ function parseWithConflict(str: string, fileLoc: string): ParseResult {
 
 export default function(str: string, fileLoc: string = 'lockfile'): ParseResult {
   str = stripBOM(str);
-  return hasMergeConflicts(str) ? parseWithConflict(str, fileLoc) : {type: 'none', object: parse(str, fileLoc)};
+  return hasMergeConflicts(str) ? parseWithConflict(str, fileLoc) : {type: 'success', object: parse(str, fileLoc)};
 }

src/lockfile/wrapper.js

@@ -3,6 +3,7 @@
 import type {Reporter} from '../reporters/index.js';
 import type {Manifest, PackageRemote} from '../types.js';
 import type {RegistryNames} from '../registries/index.js';
+import type {ParseResultType} from './parse.js';
 import {sortAlpha} from '../util/misc.js';
 import PackageRequest from '../package-request.js';
 import parse from './parse.js';
@@ -82,9 +83,12 @@ export function explodeEntry(pattern: string, obj: Object): LockManifest {
 }
 
 export default class Lockfile {
-  constructor(cache?: ?Object, source?: string) {
+  constructor(
+    {cache, source, parseResultType}: {cache?: ?Object, source?: string, parseResultType?: ParseResultType} = {},
+  ) {
     this.source = source || '';
     this.cache = cache;
+    this.parseResultType = parseResultType;
   }
 
   // source string if the `cache` was parsed
@@ -94,33 +98,36 @@ export default class Lockfile {
     [key: string]: LockManifest,
   };
 
+  parseResultType: ?ParseResultType;
+
   static async fromDirectory(dir: string, reporter?: Reporter): Promise<Lockfile> {
     // read the manifest in this directory
     const lockfileLoc = path.join(dir, constants.LOCKFILE_FILENAME);
 
     let lockfile;
     let rawLockfile = '';
+    let parseResult;
 
     if (await fs.exists(lockfileLoc)) {
       rawLockfile = await fs.readFile(lockfileLoc);
-      const lockResult = parse(rawLockfile, lockfileLoc);
+      parseResult = parse(rawLockfile, lockfileLoc);
 
       if (reporter) {
-        if (lockResult.type === 'merge') {
+        if (parseResult.type === 'merge') {
           reporter.info(reporter.lang('lockfileMerged'));
-        } else if (lockResult.type === 'conflict') {
+        } else if (parseResult.type === 'conflict') {
           reporter.warn(reporter.lang('lockfileConflict'));
         }
       }
 
-      lockfile = lockResult.object;
+      lockfile = parseResult.object;
     } else {
       if (reporter) {
         reporter.info(reporter.lang('noLockfileFound'));
       }
     }
 
-    return new Lockfile(lockfile, rawLockfile);
+    return new Lockfile({cache: lockfile, source: rawLockfile, parseResultType: parseResult && parseResult.type});
   }
 
   getLocked(pattern: string): ?LockManifest {