Graph: Remove Node interface (#6530)

* Remove Node interface * Test cleanup
parcel-bundler · Jul 1, 2021 · cd87329 · cd87329
1 parent d1e0a1a
commit cd87329
Show file tree

Hide file tree

Showing 7 changed files with 90 additions and 93 deletions.
diff --git a/packages/core/core/src/AssetGraph.js b/packages/core/core/src/AssetGraph.js
@@ -168,6 +168,16 @@ export default class AssetGraph extends ContentGraph<AssetGraphNode> {
 
   addNode(node: AssetGraphNode): NodeId {
     this.hash = null;
+    let existing = this.getNodeByContentKey(node.id);
+    if (existing != null) {
+      invariant(existing.type === node.type);
+      // $FlowFixMe[incompatible-type] Checked above
+      // $FlowFixMe[prop-missing]
+      existing.value = node.value;
+      let existingId = this.getNodeIdByContentKey(node.id);
+      this.updateNode(existingId, existing);
+      return existingId;
+    }
     return super.addNodeByContentKey(node.id, node);
   }
 

diff --git a/packages/core/core/src/ContentGraph.js b/packages/core/core/src/ContentGraph.js
@@ -1,31 +1,32 @@
 // @flow strict-local
+import type {ContentKey, NodeId} from './types';
 
 import Graph, {type GraphOpts} from './Graph';
-import type {ContentKey, Node, NodeId} from './types';
 import nullthrows from 'nullthrows';
 
-export type SerializedContentGraph<
-  TNode: Node,
-  TEdgeType: string | null = null,
-> = {|
+export type SerializedContentGraph<TNode, TEdgeType: string | null = null> = {|
   ...GraphOpts<TNode, TEdgeType>,
   _contentKeyToNodeId: Map<ContentKey, NodeId>,
+  _nodeIdToContentKey: Map<NodeId, ContentKey>,
 |};
 
 export default class ContentGraph<
-  TNode: Node,
+  TNode,
   TEdgeType: string | null = null,
 > extends Graph<TNode, TEdgeType> {
   _contentKeyToNodeId: Map<ContentKey, NodeId>;
+  _nodeIdToContentKey: Map<NodeId, ContentKey>;
 
   constructor(opts: ?SerializedContentGraph<TNode, TEdgeType>) {
     if (opts) {
-      let {_contentKeyToNodeId, ...rest} = opts;
+      let {_contentKeyToNodeId, _nodeIdToContentKey, ...rest} = opts;
       super(rest);
       this._contentKeyToNodeId = _contentKeyToNodeId;
+      this._nodeIdToContentKey = _nodeIdToContentKey;
     } else {
       super();
       this._contentKeyToNodeId = new Map();
+      this._nodeIdToContentKey = new Map();
     }
   }
 
@@ -41,21 +42,19 @@ export default class ContentGraph<
     return {
       ...super.serialize(),
       _contentKeyToNodeId: this._contentKeyToNodeId,
+      _nodeIdToContentKey: this._nodeIdToContentKey,
     };
   }
 
   addNodeByContentKey(contentKey: ContentKey, node: TNode): NodeId {
-    if (!this.hasContentKey(contentKey)) {
-      let nodeId = super.addNode(node);
-      this._contentKeyToNodeId.set(contentKey, nodeId);
-      return nodeId;
-    } else {
-      let existingNodeId = this.getNodeIdByContentKey(contentKey);
-      let existingNode = nullthrows(this.getNodeByContentKey(contentKey));
-      existingNode.value = node.value;
-      this.updateNode(existingNodeId, existingNode);
-      return existingNodeId;
+    if (this.hasContentKey(contentKey)) {
+      throw new Error('Graph already has content key ' + contentKey);
     }
+
+    let nodeId = super.addNode(node);
+    this._contentKeyToNodeId.set(contentKey, nodeId);
+    this._nodeIdToContentKey.set(nodeId, contentKey);
+    return nodeId;
   }
 
   getNodeByContentKey(contentKey: ContentKey): ?TNode {
@@ -76,9 +75,11 @@ export default class ContentGraph<
     return this._contentKeyToNodeId.has(contentKey);
   }
 
-  removeNode(nodeId: NodeId) {
+  removeNode(nodeId: NodeId): void {
     this._assertHasNodeId(nodeId);
-    this._contentKeyToNodeId.delete(nullthrows(this.getNode(nodeId)).id);
+    let contentKey = nullthrows(this._nodeIdToContentKey.get(nodeId));
+    this._contentKeyToNodeId.delete(contentKey);
+    this._nodeIdToContentKey.delete(nodeId);
     super.removeNode(nodeId);
   }
 }
diff --git a/packages/core/core/src/Graph.js b/packages/core/core/src/Graph.js
@@ -1,7 +1,7 @@
 // @flow strict-local
 
 import {toNodeId, fromNodeId} from './types';
-import type {Edge, Node, NodeId} from './types';
+import type {Edge, NodeId} from './types';
 import type {TraversalActions, GraphVisitor} from '@parcel/types';
 
 import assert from 'assert';
@@ -16,7 +16,7 @@ export type GraphOpts<TNode, TEdgeType: string | null = null> = {|
 
 export const ALL_EDGE_TYPES = '@@all_edge_types';
 
-export default class Graph<TNode: Node, TEdgeType: string | null = null> {
+export default class Graph<TNode, TEdgeType: string | null = null> {
   nodes: Map<NodeId, TNode>;
   inboundEdges: AdjacencyList<TEdgeType | null>;
   outboundEdges: AdjacencyList<TEdgeType | null>;

diff --git a/packages/core/core/src/public/MutableBundleGraph.js b/packages/core/core/src/public/MutableBundleGraph.js
@@ -88,16 +88,15 @@ export default class MutableBundleGraph extends BundleGraph<IBundle>
       entryAssetId: resolved.id,
     };
 
-    let bundleGroupNode = {
-      id: getBundleGroupId(bundleGroup),
-      type: 'bundle_group',
-      value: bundleGroup,
-    };
+    let bundleGroupKey = getBundleGroupId(bundleGroup);
+    let bundleGroupNodeId = this.#graph._graph.hasContentKey(bundleGroupKey)
+      ? this.#graph._graph.getNodeIdByContentKey(bundleGroupKey)
+      : this.#graph._graph.addNodeByContentKey(bundleGroupKey, {
+          id: bundleGroupKey,
+          type: 'bundle_group',
+          value: bundleGroup,
+        });
 
-    let bundleGroupNodeId = this.#graph._graph.addNodeByContentKey(
-      bundleGroupNode.id,
-      bundleGroupNode,
-    );
     let dependencyNodeId = this.#graph._graph.getNodeIdByContentKey(
       dependencyNode.id,
     );

diff --git a/packages/core/core/src/types.js b/packages/core/core/src/types.js
@@ -255,13 +255,6 @@ export type Edge<TEdgeType: string | null> = {|
   type: TEdgeType,
 |};
 
-export interface Node {
-  id: ContentKey;
-  +type: string;
-  // $FlowFixMe
-  value: any;
-}
-
 export type AssetNode = {|
   id: ContentKey,
   +type: 'asset',

diff --git a/packages/core/core/test/ContentGraph.test.js b/packages/core/core/test/ContentGraph.test.js
@@ -7,7 +7,7 @@ describe('ContentGraph', () => {
   it('should addNodeByContentKey if no node exists with the content key', () => {
     let graph = new ContentGraph();
 
-    const node = {id: 'contentKey', type: 'mynode', value: ' 1'};
+    const node = {};
 
     const nodeId1 = graph.addNodeByContentKey('contentKey', node);
 
@@ -16,29 +16,23 @@ describe('ContentGraph', () => {
     assert.deepEqual(graph.getNodeByContentKey('contentKey'), node);
   });
 
-  it('should update the node through addNodeByContentKey if a node with the content key exists', () => {
+  it('should throw if a node with the content key already exists', () => {
     let graph = new ContentGraph();
 
-    const node1 = {id: 'contentKey', value: '1', type: 'mynode'};
-    const node2 = {id: 'contentKey', value: '2', type: 'mynode'};
+    graph.addNodeByContentKey('contentKey', {});
 
-    const nodeId1 = graph.addNodeByContentKey('contentKey', node1);
-    const nodeId2 = graph.addNodeByContentKey('contentKey', node2);
-
-    assert.deepEqual(graph.getNode(nodeId1), node1);
-    assert(graph.hasContentKey('contentKey'));
-
-    assert.equal(nodeId1, nodeId2);
-    assert.deepEqual(graph.getNode(nodeId2), node2);
+    assert.throws(() => {
+      graph.addNodeByContentKey('contentKey', {});
+    }, /already has content key/);
   });
 
   it('should remove the content key from graph when node is removed', () => {
     let graph = new ContentGraph();
 
-    const node1 = {id: 'contentKey', value: '1', type: 'mynode'};
+    const node1 = {};
     const nodeId1 = graph.addNodeByContentKey('contentKey', node1);
 
-    assert.deepEqual(graph.getNode(nodeId1), node1);
+    assert.equal(graph.getNode(nodeId1), node1);
     assert(graph.hasContentKey('contentKey'));
 
     graph.removeNode(nodeId1);

diff --git a/packages/core/core/test/Graph.test.js b/packages/core/core/test/Graph.test.js
@@ -15,7 +15,7 @@ describe('Graph', () => {
 
   it('addNode should add a node to the graph', () => {
     let graph = new Graph();
-    let node = {id: 'do not use', type: 'mynode', value: 'a'};
+    let node = {};
     let id = graph.addNode(node);
     assert.equal(graph.nodes.get(id), node);
   });
@@ -52,40 +52,40 @@ describe('Graph', () => {
 
   it("errors when adding an edge to a node that doesn't exist", () => {
     let graph = new Graph();
-    let node = graph.addNode({id: 'foo', type: 'mynode', value: null});
+    let node = graph.addNode({});
     assert.throws(() => {
       graph.addEdge(node, toNodeId(-1));
     }, /"to" node '-1' not found/);
   });
 
   it("errors when adding an edge from a node that doesn't exist", () => {
     let graph = new Graph();
-    let node = graph.addNode({id: 'foo', type: 'mynode', value: null});
+    let node = graph.addNode({});
     assert.throws(() => {
       graph.addEdge(toNodeId(-1), node);
     }, /"from" node '-1' not found/);
   });
 
   it('hasNode should return a boolean based on whether the node exists in the graph', () => {
     let graph = new Graph();
-    let node = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
+    let node = graph.addNode({});
     assert(graph.hasNode(node));
     assert(!graph.hasNode(toNodeId(-1)));
   });
 
   it('addEdge should add an edge to the graph', () => {
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: null});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: null});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
     graph.addEdge(nodeA, nodeB);
     assert(graph.hasEdge(nodeA, nodeB));
   });
 
   it('isOrphanedNode should return true or false if the node is orphaned or not', () => {
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
-    let nodeC = graph.addNode({id: 'c', type: 'mynode', value: 'c'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
+    let nodeC = graph.addNode('c');
     graph.addEdge(nodeA, nodeB);
     graph.addEdge(nodeA, nodeC, 'edgetype');
     assert(graph.isOrphanedNode(nodeA));
@@ -100,10 +100,10 @@ describe('Graph', () => {
     //      /
     //     c
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
-    let nodeC = graph.addNode({id: 'c', type: 'mynode', value: 'c'});
-    let nodeD = graph.addNode({id: 'd', type: 'mynode', value: 'd'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
+    let nodeC = graph.addNode('c');
+    let nodeD = graph.addNode('d');
     graph.addEdge(nodeA, nodeB);
     graph.addEdge(nodeA, nodeD);
     graph.addEdge(nodeB, nodeC);
@@ -138,13 +138,13 @@ describe('Graph', () => {
     //          f
 
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
-    let nodeC = graph.addNode({id: 'c', type: 'mynode', value: 'c'});
-    let nodeD = graph.addNode({id: 'd', type: 'mynode', value: 'd'});
-    let nodeE = graph.addNode({id: 'e', type: 'mynode', value: 'e'});
-    let nodeF = graph.addNode({id: 'f', type: 'mynode', value: 'f'});
-    let nodeG = graph.addNode({id: 'g', type: 'mynode', value: 'g'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
+    let nodeC = graph.addNode('c');
+    let nodeD = graph.addNode('d');
+    let nodeE = graph.addNode('e');
+    let nodeF = graph.addNode('f');
+    let nodeG = graph.addNode('g');
 
     graph.addEdge(nodeA, nodeB);
     graph.addEdge(nodeA, nodeC);
@@ -181,13 +181,13 @@ describe('Graph', () => {
     //          f
 
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
-    let nodeC = graph.addNode({id: 'c', type: 'mynode', value: 'c'});
-    let nodeD = graph.addNode({id: 'd', type: 'mynode', value: 'd'});
-    let nodeE = graph.addNode({id: 'e', type: 'mynode', value: 'e'});
-    let nodeF = graph.addNode({id: 'f', type: 'mynode', value: 'f'});
-    let nodeG = graph.addNode({id: 'g', type: 'mynode', value: 'g'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
+    let nodeC = graph.addNode('c');
+    let nodeD = graph.addNode('d');
+    let nodeE = graph.addNode('e');
+    let nodeF = graph.addNode('f');
+    let nodeG = graph.addNode('g');
     graph.setRootNodeId(nodeA);
 
     graph.addEdge(nodeA, nodeB);
@@ -216,11 +216,11 @@ describe('Graph', () => {
     //       \ /    |
     //        e -----
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
-    let nodeC = graph.addNode({id: 'c', type: 'mynode', value: 'c'});
-    let nodeD = graph.addNode({id: 'd', type: 'mynode', value: 'd'});
-    let nodeE = graph.addNode({id: 'e', type: 'mynode', value: 'e'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
+    let nodeC = graph.addNode('c');
+    let nodeD = graph.addNode('d');
+    let nodeE = graph.addNode('e');
     graph.setRootNodeId(nodeA);
 
     graph.addEdge(nodeA, nodeB);
@@ -248,8 +248,8 @@ describe('Graph', () => {
   it('removing a node with only one inbound edge does not cause it to be removed as an orphan', () => {
     let graph = new Graph();
 
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
     graph.setRootNodeId(nodeA);
 
     graph.addEdge(nodeA, nodeB);
@@ -266,13 +266,13 @@ describe('Graph', () => {
 
   it("replaceNodeIdsConnectedTo should update a node's downstream nodes", () => {
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
-    let nodeC = graph.addNode({id: 'c', type: 'mynode', value: 'c'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
+    let nodeC = graph.addNode('c');
     graph.addEdge(nodeA, nodeB);
     graph.addEdge(nodeA, nodeC);
 
-    let nodeD = graph.addNode({id: 'd', type: 'mynode', value: 'd'});
+    let nodeD = graph.addNode('d');
     graph.replaceNodeIdsConnectedTo(nodeA, [nodeB, nodeD]);
 
     assert(graph.hasNode(nodeA));
@@ -287,10 +287,10 @@ describe('Graph', () => {
 
   it('traverses along edge types if a filter is given', () => {
     let graph = new Graph();
-    let nodeA = graph.addNode({id: 'a', type: 'mynode', value: 'a'});
-    let nodeB = graph.addNode({id: 'b', type: 'mynode', value: 'b'});
-    let nodeC = graph.addNode({id: 'c', type: 'mynode', value: 'c'});
-    let nodeD = graph.addNode({id: 'd', type: 'mynode', value: 'd'});
+    let nodeA = graph.addNode('a');
+    let nodeB = graph.addNode('b');
+    let nodeC = graph.addNode('c');
+    let nodeD = graph.addNode('d');
 
     graph.addEdge(nodeA, nodeB, 'edgetype');
     graph.addEdge(nodeA, nodeD);