improve readability of the error output

[giniedp]

I have following suggestions to improve the readability of the error output

1. The base statement. Right now if webpack spits out the warnings it reads like this WARNING in Circular dependency detected: i'd suggest to use the name of the plugin, like this WARNING in [circular-dependency-plugin]: This however does not improve readability. Its just a personal preference.

2. The line length. Right now, all paths are written into a single line joined with " -> ".

   frontend/modules/foo/foo.js -> frontend/modules/bar/bar.js -> frontend/modules/baz/baz.js

   This is hard to read when there are many paths inside the circle. My suggestion is to write each path in its own line. Maybe indent with a TAB and add an utf8 arrow symbol, like this

   new Error(["[circular dependency]", ...circularPath].join("\r\n\t↳ "))

   which renders like this

   WARNING in [circular-dependency-plugin]
   ↳ frontend/modules/foo/foo.js
   ↳ frontend/modules/bar/bar.js
   ↳ frontend/modules/baz/baz.js

3. Report detected circles once. Right now, each circle is reported at least twice (for each file involved in the circle). The plugin could track already detected circles and then check each detected new circle against that list.

   function isAlreadyTracked(trackedList, detected) {
   for (const circle of trackedList) {
   if (circle.length !== detected.length) {
     continue
   }
   const index = circle.indexOf(detected[0])
   if (index === -1) {
     continue
   }
   const len = detected.length - 1 // first and last entries are the same, skip the last
   return detected.every((it, i) => circle[(i + index) % len] === it)
   }
   return false
   }

I might open a pull request, but that might take me some time. I thought i'd share my ideas up front.

Great plugin, helped me a lot.

[aackerman]

I'll take this into consideration, but I'm not changing the base output right now. In v4.x I've provided the onDetected method as an escape hatch for customization.

Please take a look at that and let me know if it doesn't resolve your concerns.

[giniedp]

i actually use my own implementation, based on your source. My intention was to share my improvements which i think some devs would be happy to see. Actually points 2. and 3. are the main reasons why i had to roll up my own implementation.

Points 1. and 2. are solvable with onDetected. That callback is nice to have for little adjustments like the log format. But point 3. is a bit more complicated. One would have to track all previously detected circular dependencies and also reset that information before a rebuild (watch mode). From that callback only one does not know when to reset the tracked information. Further, that kind of logic would clutter the configuration file. That should happen inside the plugin.

[aackerman]

@giniedp do you have a specific example where a circular dependency is reported more than one time?

[giniedp]

if foo.ts and bar.ts both require each other, then the plugin reports 2 detections

foo.ts -> bar.ts -> foo.ts

and

bar.ts -> foo.ts -> bar.ts

[Retsam]

I'm seeing the same behavior, if I have N files in a circular reference, I'll see N lines of output for the chain.

Here's a gist with a full example: https://gist.github.com/Retsam/9c35799c3b1867d9a1170f8c04301308, and here's the output I get when I webpack it:

WARNING in Circular dependency detected:
a.js -> b.js -> c.js -> a.js

WARNING in Circular dependency detected:
b.js -> c.js -> a.js -> b.js

WARNING in Circular dependency detected:
c.js -> a.js -> b.js -> c.js