fixes lockfile inconsistency between runs (#109)

* fixes lockfile inconsistency between runs

* nits and comments for Shayne

Author: bestander

src/lockfile/index.js

@@ -114,12 +114,19 @@ export default class Lockfile {
     [packagePattern: string]: Manifest
   }): Object {
     let lockfile = {};
-    let seen: Map<Manifest, string> = new Map;
-
-    for (let pattern in patterns) {
+    let seen: Map<string, string> = new Map;
+    // order by name so that lockfile manifest is assigned to the first dependency with this manifest
+    // the others that have the same remote.resovled will just refer to the first
+    // oredring allows for consistency in lockfile when it is serialized
+    let sortedPatternsKeys: string[] = Object.keys(patterns).sort((a, b) => {
+      return a.toLowerCase().localeCompare(b.toLowerCase());
+    });
+
+    for (let pattern of sortedPatternsKeys) {
       let pkg = patterns[pattern];
+      let remote = pkg.remote;
 
-      let seenPattern = seen.get(pkg);
+      let seenPattern = remote && remote.resolved && seen.get(remote.resolved);
       if (seenPattern) {
         // no point in duplicating it
         lockfile[pattern] = seenPattern;
@@ -129,7 +136,6 @@ export default class Lockfile {
       let ref = pkg.reference;
       invariant(ref, "Package is missing a reference");
 
-      let remote = pkg.remote;
       invariant(remote, "Package is missing a remote");
 
       lockfile[pattern] = {
@@ -143,7 +149,9 @@ export default class Lockfile {
         permissions: _.isEmpty(ref.permissions) ? undefined : ref.permissions
       };
 
-      seen.set(pkg, pattern);
+      if (remote.resolved) {
+        seen.set(remote.resolved, pattern);
+      }
     }
 
     return lockfile;

src/lockfile/stringify.js

@@ -9,6 +9,8 @@
  * @flow
  */
 
+let _ = require("lodash");
+
 function shouldWrapKey(str: string): boolean {
   return str.indexOf("true") === 0 || str.indexOf("false") === 0 ||
          /[:\s\n\\"]/g.test(str) || /^[0-9]/g.test(str) || !/^[a-zA-Z]/g.test(str);
@@ -42,13 +44,17 @@ export default function stringify(obj: any, indent: string = ""): string {
 
   let lines = [];
 
-  let keys = Object.keys(obj).sort(function (a, b) {
-    // sort alphabetically
-    return a.toLowerCase().localeCompare(b.toLowerCase());
-  }).sort(function (a, b) {
-    // prioritise certain fields
-    return +(getKeyPriority(a) > getKeyPriority(b));
-  });
+  // Sorting order needs to be consistent between runs, we run native sort by name because there are no
+  // problems with it being unstable because there are no to keys the same
+  // However priorities can be duplicated and native sort can shuffle things from run to run
+  let keys = Object.keys(obj)
+    .sort(function (a, b) {
+      // sort alphabetically
+      return a.toLowerCase().localeCompare(b.toLowerCase());
+    });
+
+  // stable sort, V8 Array.prototype.sort is not stable and we don't want to shuffle things randomly
+  keys = _.sortBy(keys, getKeyPriority);
 
   for (let key of keys) {
     let val = obj[key];

test/lockfile.js

@@ -116,7 +116,7 @@ test("Lockfile.getLockfile", (t) => {
         permissions: {}
       },
       remote: {
-        resolved: "http://example.com",
+        resolved: "http://example.com/foobar",
         registry: "npm"
       }
     },
@@ -137,7 +137,7 @@ test("Lockfile.getLockfile", (t) => {
         }
       },
       remote: {
-        resolved: "http://example.com",
+        resolved: "http://example.com/barfoo",
         registry: "bower"
       }
     }
@@ -152,7 +152,7 @@ test("Lockfile.getLockfile", (t) => {
       name: "foobar",
       version: "0.0.0",
       uid: undefined,
-      resolved: "http://example.com",
+      resolved: "http://example.com/foobar",
       registry: undefined,
       dependencies: undefined,
       optionalDependencies: undefined,
@@ -163,7 +163,7 @@ test("Lockfile.getLockfile", (t) => {
       name: "barfoo",
       version: "0.0.1",
       uid: "0.1.0",
-      resolved: "http://example.com",
+      resolved: "http://example.com/barfoo",
       registry: "bower",
       dependencies: { yes: "no" },
       optionalDependencies: { no: "yes" },
@@ -175,3 +175,43 @@ test("Lockfile.getLockfile", (t) => {
 
   t.deepEqual(actual, expected);
 });
+
+test("Lockfile.getLockfile (sorting)", (t) => {
+  let patterns = {
+    foobar2: {
+      name: "foobar",
+      version: "0.0.0",
+      uid: "0.0.0",
+      dependencies: {},
+      optionalDependencies: {},
+      reference: {
+        permissions: {}
+      },
+      remote: {
+        resolved: "http://example.com/foobar",
+        registry: "npm"
+      }
+    },
+  };
+
+  patterns.foobar1 = patterns.foobar2;
+
+  let actual = new Lockfile().getLockfile(patterns);
+
+  let expected = {
+    foobar1: {
+      name: "foobar",
+      version: "0.0.0",
+      uid: undefined,
+      resolved: "http://example.com/foobar",
+      registry: undefined,
+      dependencies: undefined,
+      optionalDependencies: undefined,
+      permissions: undefined
+    },
+
+    foobar2: "foobar1"
+  };
+
+  t.deepEqual(actual, expected);
+});