Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Graph: Remove Node interface #6530

Merged
merged 2 commits into from
Jul 1, 2021
Merged

Graph: Remove Node interface #6530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df87d05
Remove Node interface
Jun 29, 2021
f2a8a92
Test cleanup
Jun 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/core/core/src/AssetGraph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
39 changes: 20 additions & 19 deletions packages/core/core/src/ContentGraph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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();
}
}

Expand All @@ -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 {
Expand All @@ -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);
}
}
4 changes: 2 additions & 2 deletions packages/core/core/src/Graph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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>;
Expand Down
17 changes: 8 additions & 9 deletions packages/core/core/src/public/MutableBundleGraph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
7 changes: 0 additions & 7 deletions packages/core/core/src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
22 changes: 8 additions & 14 deletions packages/core/core/test/ContentGraph.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);
Expand Down
84 changes: 42 additions & 42 deletions packages/core/core/test/Graph.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
Expand Down Expand Up @@ -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));
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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));
Expand All @@ -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);
Expand Down