Skip to content

Commit

Permalink
Switch to use Map/Set instead of raw Objects
Browse files Browse the repository at this point in the history
  • Loading branch information
@jriecken
jriecken committed Dec 5, 2023
1 parent 4093b79 commit c9238dc
Show file tree
Hide file tree
Showing 4 changed files with 487 additions and 206 deletions.
104 changes: 51 additions & 53 deletions lib/dep_graph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,18 @@
* Detects cycles and throws an Error if one is detected (unless the "circular"
* parameter is "true" in which case it ignores them).
*
* @param edges The set of edges to DFS through
* @param edges The edges to DFS through (this is a Map of node to Array of nodes)
* @param leavesOnly Whether to only return "leaf" nodes (ones who have no edges)
* @param result An array in which the results will be populated
* @param circular A boolean to allow circular dependencies
*/
function createDFS(edges, leavesOnly, result, circular) {
var visited = {};
var visited = new Set();
return function (start) {
if (visited[start]) {
if (visited.has(start)) {
return;
}
var inCurrentPath = {};
var inCurrentPath = new Set();
var currentPath = [];
var todo = []; // used as a stack
todo.push({ node: start, processed: false });
Expand All @@ -29,10 +29,10 @@ function createDFS(edges, leavesOnly, result, circular) {
var node = current.node;
if (!processed) {
// Haven't visited edges yet (visiting phase)
if (visited[node]) {
if (visited.has(node)) {
todo.pop();
continue;
} else if (inCurrentPath[node]) {
} else if (inCurrentPath.has(node)) {
// It's not a DAG
if (circular) {
todo.pop();
Expand All @@ -43,9 +43,9 @@ function createDFS(edges, leavesOnly, result, circular) {
throw new DepGraphCycleError(currentPath);
}

inCurrentPath[node] = true;
inCurrentPath.add(node);
currentPath.push(node);
var nodeEdges = edges[node];
var nodeEdges = edges.get(node);
// (push edges onto the todo stack in reverse order to be order-compatible with the old DFS implementation)
for (var i = nodeEdges.length - 1; i >= 0; i--) {
todo.push({ node: nodeEdges[i], processed: false });
Expand All @@ -55,9 +55,9 @@ function createDFS(edges, leavesOnly, result, circular) {
// Have visited edges (stack unrolling phase)
todo.pop();
currentPath.pop();
inCurrentPath[node] = false;
visited[node] = true;
if (!leavesOnly || edges[node].length === 0) {
inCurrentPath.delete(node);
visited.add(node);
if (!leavesOnly || edges.get(node).length === 0) {
result.push(node);
}
}
Expand All @@ -69,17 +69,17 @@ function createDFS(edges, leavesOnly, result, circular) {
* Simple Dependency Graph
*/
var DepGraph = (exports.DepGraph = function DepGraph(opts) {
this.nodes = {}; // Node -> Node/Data (treated like a Set)
this.outgoingEdges = {}; // Node -> [Dependency Node]
this.incomingEdges = {}; // Node -> [Dependant Node]
this.nodes = new Map(); // Node -> Node/Data
this.outgoingEdges = new Map(); // Node -> [Dependency Node]
this.incomingEdges = new Map(); // Node -> [Dependant Node]
this.circular = opts && !!opts.circular; // Allows circular deps
});
DepGraph.prototype = {
/**
* The number of nodes in the graph.
*/
size: function () {
return Object.keys(this.nodes).length;
return this.nodes.size;
},
/**
* Add a node to the dependency graph. If a node already exists, this method will do nothing.
Expand All @@ -88,44 +88,44 @@ DepGraph.prototype = {
if (!this.hasNode(node)) {
// Checking the arguments length allows the user to add a node with undefined data
if (arguments.length === 2) {
this.nodes[node] = data;
this.nodes.set(node, data);
} else {
this.nodes[node] = node;
this.nodes.set(node, node);
}
this.outgoingEdges[node] = [];
this.incomingEdges[node] = [];
this.outgoingEdges.set(node, []);
this.incomingEdges.set(node, []);
}
},
/**
* Remove a node from the dependency graph. If a node does not exist, this method will do nothing.
*/
removeNode: function (node) {
if (this.hasNode(node)) {
delete this.nodes[node];
delete this.outgoingEdges[node];
delete this.incomingEdges[node];
this.nodes.delete(node);
this.outgoingEdges.delete(node);
this.incomingEdges.delete(node);
[this.incomingEdges, this.outgoingEdges].forEach(function (edgeList) {
Object.keys(edgeList).forEach(function (key) {
var idx = edgeList[key].indexOf(node);
edgeList.forEach(function (v) {
var idx = v.indexOf(node);
if (idx >= 0) {
edgeList[key].splice(idx, 1);
v.splice(idx, 1);
}
}, this);
});
});
}
},
/**
* Check if a node exists in the graph
*/
hasNode: function (node) {
return this.nodes.hasOwnProperty(node);
return this.nodes.has(node);
},
/**
* Get the data associated with a node name
*/
getNodeData: function (node) {
if (this.hasNode(node)) {
return this.nodes[node];
return this.nodes.get(node);
} else {
throw new Error("Node does not exist: " + node);
}
Expand All @@ -135,7 +135,7 @@ DepGraph.prototype = {
*/
setNodeData: function (node, data) {
if (this.hasNode(node)) {
this.nodes[node] = data;
this.nodes.set(node, data);
} else {
throw new Error("Node does not exist: " + node);
}
Expand All @@ -151,11 +151,11 @@ DepGraph.prototype = {
if (!this.hasNode(to)) {
throw new Error("Node does not exist: " + to);
}
if (this.outgoingEdges[from].indexOf(to) === -1) {
this.outgoingEdges[from].push(to);
if (this.outgoingEdges.get(from).indexOf(to) === -1) {
this.outgoingEdges.get(from).push(to);
}
if (this.incomingEdges[to].indexOf(from) === -1) {
this.incomingEdges[to].push(from);
if (this.incomingEdges.get(to).indexOf(from) === -1) {
this.incomingEdges.get(to).push(from);
}
return true;
},
Expand All @@ -165,16 +165,16 @@ DepGraph.prototype = {
removeDependency: function (from, to) {
var idx;
if (this.hasNode(from)) {
idx = this.outgoingEdges[from].indexOf(to);
idx = this.outgoingEdges.get(from).indexOf(to);
if (idx >= 0) {
this.outgoingEdges[from].splice(idx, 1);
this.outgoingEdges.get(from).splice(idx, 1);
}
}

if (this.hasNode(to)) {
idx = this.incomingEdges[to].indexOf(from);
idx = this.incomingEdges.get(to).indexOf(from);
if (idx >= 0) {
this.incomingEdges[to].splice(idx, 1);
this.incomingEdges.get(to).splice(idx, 1);
}
}
},
Expand All @@ -185,13 +185,12 @@ DepGraph.prototype = {
clone: function () {
var source = this;
var result = new DepGraph();
var keys = Object.keys(source.nodes);
keys.forEach(function (n) {
result.nodes[n] = source.nodes[n];
result.outgoingEdges[n] = source.outgoingEdges[n].slice(0);
result.incomingEdges[n] = source.incomingEdges[n].slice(0);
source.nodes.forEach(function (v, n) {
result.nodes.set(n, v);
result.outgoingEdges.set(n, source.outgoingEdges.get(n).slice(0));
result.incomingEdges.set(n, source.incomingEdges.get(n).slice(0));
});
result.circular = source.circular
result.circular = source.circular;
return result;
},
/**
Expand All @@ -201,7 +200,7 @@ DepGraph.prototype = {
*/
directDependenciesOf: function (node) {
if (this.hasNode(node)) {
return this.outgoingEdges[node].slice(0);
return this.outgoingEdges.get(node).slice(0);
} else {
throw new Error("Node does not exist: " + node);
}
Expand All @@ -213,7 +212,7 @@ DepGraph.prototype = {
*/
directDependantsOf: function (node) {
if (this.hasNode(node)) {
return this.incomingEdges[node].slice(0);
return this.incomingEdges.get(node).slice(0);
} else {
throw new Error("Node does not exist: " + node);
}
Expand Down Expand Up @@ -281,7 +280,7 @@ DepGraph.prototype = {
overallOrder: function (leavesOnly) {
var self = this;
var result = [];
var keys = Object.keys(this.nodes);
var keys = Array.from(this.nodes.keys());
if (keys.length === 0) {
return result; // Empty graph
} else {
Expand All @@ -304,7 +303,7 @@ DepGraph.prototype = {
// run a DFS starting at these points to get the order
keys
.filter(function (node) {
return self.incomingEdges[node].length === 0;
return self.incomingEdges.get(node).length === 0;
})
.forEach(function (n) {
DFS(n);
Expand All @@ -331,11 +330,10 @@ DepGraph.prototype = {
*/
entryNodes: function () {
var self = this;
return Object.keys(this.nodes).filter(function (node) {
return self.incomingEdges[node].length === 0;
return Array.from(this.nodes.keys()).filter(function (node) {
return self.incomingEdges.get(node).length === 0;
});
}

},
};

// Create some aliases
Expand All @@ -360,7 +358,7 @@ DepGraphCycleError.prototype = Object.create(Error.prototype, {
value: Error,
enumerable: false,
writable: true,
configurable: true
}
configurable: true,
},
});
Object.setPrototypeOf(DepGraphCycleError, Error);
Loading

3 comments on commit c9238dc

@ghostfreak3000
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to bring to your attention that this code :

const { DepGraph } = require("dependency-graph");

const graph = new DepGraph();

graph.addNode("a");
graph.addNode("b");
graph.addNode("c");
graph.addNode("d");
graph.addNode("e");

graph.addDependency("c", "b");
graph.addDependency("d", "c");
graph.addDependency("d", "a");
graph.addDependency("e", "d");

console.log(JSON.stringify(graph));

Produces this output

{"nodes":{},"outgoingEdges":{},"incomingEdges":{}}

Due to JSON.stringify having some quirks with Map...
I assume many individuals (myself included) use JSON.stringify to serialize to json objects for the purpose of transmitting over the wire.
i already found a work around for this using yahoo's serialize-javascript lib.. but maybe providing a toJson/fromJson methods for marshalling and unmarshalling would be useful..

if this is alright will try to create a PR over the weekend or something

@jriecken
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this wasn't an intentional change (although there's never really been a promise that the graph is JSON-serializable)

The only thing I worry about adding an official toJSON() / fromJSON(string) is that it's possible for someone who is using this library to add a node that has data attached that is not JSON.stringifyable (e.g. as you've shown a Map, or a Function, etc) and it's not clear what the output should be (fromJSON(graph.toJSON()) would not return the same thing)

@ghostfreak3000
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, no need to add functions that work depending on domain context.
Also I suspect by the time someone needs a graphlib and is working with data niche enough that JSON.stringify doesn't work, they can easily write a serialiser for their context.
thanks for this lib otherwise, really saved my skin when I needed to topologically sort some data arranged in a dependency tree

Please sign in to comment.