[V3] Fix build indeterminism by sorting entries (#319)

Author: mattcompiles

crates/atlaspack/src/requests/asset_graph_request.rs

@@ -55,6 +55,7 @@ struct AssetGraphBuilder {
   receiver: ResultReceiver,
   asset_request_to_asset_idx: HashMap<u64, NodeIndex>,
   waiting_asset_requests: HashMap<u64, HashSet<NodeIndex>>,
+  entry_dependencies: Vec<(String, NodeIndex)>,
 }
 
 impl AssetGraphBuilder {
@@ -71,6 +72,7 @@ impl AssetGraphBuilder {
       receiver,
       asset_request_to_asset_idx: HashMap::new(),
       waiting_asset_requests: HashMap::new(),
+      entry_dependencies: Vec::new(),
     }
   }
 
@@ -131,6 +133,22 @@ impl AssetGraphBuilder {
       }
     }
 
+    // Connect the entries to the root node in the graph. We do this in
+    // alphabetical order so it's consistent between builds.
+    //
+    // Ideally, we wouldn't depend on edge order being consistent between builds
+    // and instead rely on in-place sorting or similar to ensure deterministic
+    // builds. However, as the rest of the code base (bundling, runtimes,
+    // packaging, etc) relies on the deterministic edge order and it's very
+    // complicated/risky to fix all the places that would be affected we'll keep it that
+    // way for now.
+    self
+      .entry_dependencies
+      .sort_by_key(|(entry, _)| entry.clone());
+    for (_, node_index) in self.entry_dependencies.iter() {
+      self.graph.add_edge(&self.graph.root_node(), node_index);
+    }
+
     Ok(ResultAndInvalidations {
       result: RequestResult::AssetGraph(AssetGraphRequestOutput { graph: self.graph }),
       invalidations: vec![],
@@ -192,10 +210,12 @@ impl AssetGraphBuilder {
         .request_context
         .queue_request(asset_request, self.sender.clone());
     } else if let Some(asset_node_index) = self.asset_request_to_asset_idx.get(&id) {
-      // We have already completed this AssetRequest so we can connect the
-      // Dependency to the Asset immediately
-      self.graph.add_edge(&dependency_idx, asset_node_index);
-      self.propagate_requested_symbols(*asset_node_index, dependency_idx);
+      if !self.graph.has_edge(&dependency_idx, asset_node_index) {
+        // We have already completed this AssetRequest so we can connect the
+        // Dependency to the Asset immediately
+        self.graph.add_edge(&dependency_idx, asset_node_index);
+        self.propagate_requested_symbols(*asset_node_index, dependency_idx);
+      }
     } else {
       // The AssetRequest has already been kicked off but is yet to
       // complete. Register this Dependency to be connected once it
@@ -282,8 +302,10 @@ impl AssetGraphBuilder {
     // for this AssetNode to be created
     if let Some(waiting) = self.waiting_asset_requests.remove(&request_id) {
       for dep in waiting {
-        self.graph.add_edge(&dep, &asset_idx);
-        self.propagate_requested_symbols(asset_idx, dep);
+        if !self.graph.has_edge(&dep, &asset_idx) {
+          self.graph.add_edge(&dep, &asset_idx);
+          self.propagate_requested_symbols(asset_idx, dep);
+        }
       }
     }
   }
@@ -405,10 +427,12 @@ impl AssetGraphBuilder {
     for target in targets {
       let entry =
         diff_paths(&entry, &self.request_context.project_root).unwrap_or_else(|| entry.clone());
+      let entry = entry.to_str().unwrap().to_string();
 
-      let dependency = Dependency::entry(entry.to_str().unwrap().to_string(), target);
+      let dependency = Dependency::entry(entry.clone(), target);
 
       let dep_node = self.graph.add_entry_dependency(dependency.clone());
+      self.entry_dependencies.push((entry, dep_node));
 
       let request = PathRequest {
         dependency: Arc::new(dependency),

crates/atlaspack_core/src/asset_graph/asset_graph.rs

@@ -175,7 +175,6 @@ impl AssetGraph {
   pub fn add_entry_dependency(&mut self, dependency: Dependency) -> NodeIndex {
     let is_library = dependency.env.is_library;
     let dependency_idx = self.add_dependency(dependency);
-    self.add_edge(&self.root_node_index.clone(), &dependency_idx);
 
     if is_library {
       if let Some(dependency_node) = self.get_dependency_node_mut(&dependency_idx) {
@@ -186,6 +185,10 @@ impl AssetGraph {
     dependency_idx
   }
 
+  pub fn has_edge(&mut self, from_idx: &NodeIndex, to_idx: &NodeIndex) -> bool {
+    self.graph.contains_edge(*from_idx, *to_idx)
+  }
+
   pub fn add_edge(&mut self, from_idx: &NodeIndex, to_idx: &NodeIndex) {
     self.graph.add_edge(*from_idx, *to_idx, ());
   }

crates/node-bindings/src/atlaspack/serialize_asset_graph.rs

@@ -12,6 +12,7 @@ use atlaspack_core::types::{Asset, Dependency};
 ///   nodes: Array<JsBuffer>,
 /// }
 /// ```
+#[tracing::instrument(level = "info", skip_all)]
 pub fn serialize_asset_graph(env: &Env, asset_graph: &AssetGraph) -> anyhow::Result<JsObject> {
   // Serialize graph nodes in parallel
   let nodes = asset_graph

packages/core/integration-tests/test/bundler.js

@@ -2341,4 +2341,60 @@ describe('bundler', function () {
       await run(splitBundle);
     },
   );
+
+  it('should produce deterministic builds with multiple entries', async () => {
+    await fsFixture(overlayFS, __dirname)`
+        deterministic-builds
+          shared-one.js:
+            export const one = 'one';
+          shared-two.js:
+            export const two = 'two';
+          one.js:
+            import {one} from './shared-one';
+            import {two} from './shared-two';
+            sideEffectNoop(one + two);
+          entry-one.html:
+            <script type="module" src="./one.js" />
+          two.js:
+            import {two} from './shared-two';
+            import {one} from './shared-one';
+            sideEffectNoop(two + one);
+          entry-two.html:
+            <script type="module" src="./two.js" />
+          package.json:
+            {
+              "@atlaspack/bundler-default": {
+                "minBundleSize": 0,
+                "manualSharedBundles": [{
+                  "name": "shared",
+                  "assets": ["shared*.js"]
+                }]
+              }
+            }
+          yarn.lock:
+      `;
+
+    let build = () =>
+      bundle(
+        [
+          path.join(__dirname, 'deterministic-builds/entry-one.html'),
+          path.join(__dirname, 'deterministic-builds/entry-two.html'),
+        ],
+        {
+          inputFS: overlayFS,
+        },
+      ).then((b) =>
+        b
+          .getBundles()
+          .map((b) => b.filePath)
+          .sort(),
+      );
+
+    let bundles = await build();
+
+    for (let i = 0; i < 10; i++) {
+      let compareBundles = await build();
+      assert.deepEqual(bundles, compareBundles);
+    }
+  });
 });

packages/core/integration-tests/test/scope-hoisting.js

@@ -6177,8 +6177,11 @@ describe('scope hoisting', function () {
     let contents = await outputFS.readFile(indexBundle.filePath, 'utf8');
     assert(contents.includes('$parcel$global.rwr('));
 
+    let indexHtmlBundle = nullthrows(
+      b.getBundles().find((b) => /index.*\.html/.test(b.filePath)),
+    );
     let result;
-    await run(b, {
+    await runBundle(b, indexHtmlBundle, {
       result: (r) => {
         result = r;
       },