Make insertion of modules into the delta structure deterministic

Summary:
After checking more deeply, there were still situations where the delta bundler could generate different bundles between runs: when a module is required by two different modules (it has two or more inverse dependencies), that module would not always be inserted after the first inverse dependency in the bundle, which would cause some bundling order discrepancies.

In order to fix that, the bundler now re-traverses synchronously the dependency graph using a DFS traversing algorithm to guarantee the same order. This will add some small runtime overhead (specially when generating the initial bundle), but it's not worrying (from benchmarks in my macbook pro it'll add ~10ms of initial build for every 1000 modules traversed, being this overhead linear).

Reviewed By: mjesun

Differential Revision: D6124993

fbshipit-source-id: 9bc7cb329f01a7860c7d3b52c3376c643ea5cf3b

packages/metro-bundler/src/DeltaBundler/__tests__/traverseDependencies-test.js

@@ -26,6 +26,13 @@ let dependencyGraph;
 let mockedDependencies;
 let mockedDependencyTree;
 
+function deferred(value) {
+  let resolve;
+  const promise = new Promise(res => (resolve = res));
+
+  return {promise, resolve: () => resolve(value)};
+}
+
 function createModule({path, name, isAsset, isJSON}) {
   return {
     path,
@@ -207,4 +214,69 @@ describe('edge cases', () => {
       deleted: new Set(['/baz']),
     });
   });
+
+  it('should traverse the dependency tree in a deterministic order', async () => {
+    // Mocks the shallow dependency call, always resolving the module in
+    // `slowPath` after the module in `fastPath`.
+    function mockShallowDependencies(slowPath, fastPath) {
+      let deferredSlow;
+      let fastResolved = false;
+
+      dependencyGraph.getShallowDependencies = async path => {
+        const deps = mockedDependencyTree.get(path);
+
+        const result = deps
+          ? await Promise.all(deps.map(dep => dep.getName()))
+          : [];
+
+        if (path === slowPath && !fastResolved) {
+          // Return a Promise that won't be resolved after fastPath.
+          deferredSlow = deferred(result);
+          return deferredSlow.promise;
+        }
+
+        if (path === fastPath) {
+          fastResolved = true;
+
+          if (deferredSlow) {
+            return new Promise(async resolve => {
+              await resolve(result);
+
+              deferredSlow.resolve();
+            });
+          }
+        }
+
+        return result;
+      };
+    }
+
+    async function assertOrder() {
+      expect(
+        Array.from(
+          (await initialTraverseDependencies(
+            '/bundle',
+            dependencyGraph,
+            {},
+            new Map(),
+          )).added,
+        ),
+      ).toEqual(['/foo', '/baz', '/bar']);
+    }
+
+    // Create a dependency tree where moduleBaz has two inverse dependencies.
+    mockedDependencyTree = new Map([
+      [entryModule.path, [moduleFoo, moduleBar]],
+      [moduleFoo.path, [moduleBaz]],
+      [moduleBar.path, [moduleBaz]],
+    ]);
+
+    // Test that even when having different modules taking longer, the order
+    // remains the same.
+    mockShallowDependencies('/foo', '/bar');
+    await assertOrder();
+
+    mockShallowDependencies('/bar', '/foo');
+    await assertOrder();
+  });
 });

packages/metro-bundler/src/DeltaBundler/traverseDependencies.js

@@ -141,34 +141,30 @@ async function traverseDependenciesForSingleFile(
     Array.from(
       currentDependencies,
     ).map(async ([absolutePath, relativePath]) => {
-      let newDependencies;
-
-      if (!previousDependencies.has(absolutePath)) {
-        newDependencies = await addDependency(
-          nonNullEdge,
-          relativePath,
-          dependencyGraph,
-          transformOptions,
-          edges,
-          () => {
-            total++;
-            onProgress(numProcessed, total);
-          },
-          () => {
-            numProcessed++;
-            onProgress(numProcessed, total);
-          },
-        );
-      } else {
-        newDependencies = new Set();
+      if (previousDependencies.has(absolutePath)) {
+        return new Set();
       }
 
-      return newDependencies;
+      return await addDependency(
+        nonNullEdge,
+        relativePath,
+        dependencyGraph,
+        transformOptions,
+        edges,
+        () => {
+          total++;
+          onProgress(numProcessed, total);
+        },
+        () => {
+          numProcessed++;
+          onProgress(numProcessed, total);
+        },
+      );
     }),
   );
 
   return {
-    added: flatten(added),
+    added: flatten(reorderDependencies(added, edges)),
     deleted: flatten(deleted),
   };
 }
@@ -329,6 +325,42 @@ function resolveDependencies(
   );
 }
 
+/**
+ * Retraverse the dependency graph in DFS order to reorder the modules and
+ * guarantee the same order between runs.
+ */
+function reorderDependencies(
+  dependencies: Array<Set<string>>,
+  edges: DependencyEdges,
+): Array<Set<string>> {
+  const flatDependencies = flatten(dependencies);
+
+  return dependencies.map(dependencies =>
+    reorderDependency(Array.from(dependencies)[0], flatDependencies, edges),
+  );
+}
+
+function reorderDependency(
+  path: string,
+  dependencies: Set<string>,
+  edges: DependencyEdges,
+  orderedDependencies?: Set<string> = new Set(),
+): Set<string> {
+  const edge = edges.get(path);
+
+  if (!edge || !dependencies.has(path) || orderedDependencies.has(path)) {
+    return orderedDependencies;
+  }
+
+  orderedDependencies.add(path);
+
+  edge.dependencies.forEach(path =>
+    reorderDependency(path, dependencies, edges, orderedDependencies),
+  );
+
+  return orderedDependencies;
+}
+
 function flatten<T>(input: Iterable<Iterable<T>>): Set<T> {
   const output = new Set();