jriecken / dependency-graph

A simple dependency graph for Node.js
http://jriecken.github.io/dependency-graph/
MIT License
333 stars 49 forks source link

Allows for circular dependencies #21

Closed tbranyen closed 6 years ago

tbranyen commented 6 years ago

When working with ES Module dependencies, this graph is very handy. I'd like to expand it to support circular dependencies, which aren't an issue in this realm.

I don't think this API is the most ideal, since it tacks on yet another argument to all the calls. I didn't add tests yet, since I wanted to gauge your opinion.

jriecken commented 6 years ago

A dependency graph has to be a DAG (Directed Acyclic Graph). If there are any cycles there's not really any way to produce a topological sort (i.e. the overall order).

E.g. if you have

A -> B -> C
^---------v

There's no way to satisfy the dependencies of A because it (eventually) depends on C, which depends on A

tbranyen commented 6 years ago

@jriecken Makes sense. I'm using this module outside of your intended use case, as it's especially useful for my ES module bundler. I can fully normalize every dependency and get an accurate order back for inlining into a single file. My thought here is that I'm intentionally breaking the guarantees afforded by no cyclic dependencies. So far I haven't seen any evidence that the overall order produced by my patch will cause any issues.

If you have interest in supporting my use case (module bundling) I'll research why my patch works for cyclic modules (valid in ES6 specification). My guess is that the algorithm for determining overall order with cyclic dependencies goes with first occurrence.

jriecken commented 6 years ago

That's true - I guess as long as it outputs a potentially valid (assuming the cycle can be resolved by whatever is using the output) order and doesn't infinitely loop, an additional option to allow cycles would work.

A better place for the option would probably be a constructor parameter for DepGraph rather than in all the methods?

tbranyen commented 6 years ago

@jriecken I agree, I'll make that change and add a unit test

tbranyen commented 6 years ago

I added some extra content into the README, lemme know if it needs any revising.

jriecken commented 6 years ago

Released 0.7.0 with these changes in it