Circular dependencies in media modules

[apapko]

Found using webpack CircularDependencyPlugin ( "circular-dependency-plugin": "^1.1.0" )
Manually looked into Media files and indeed they require each other.

Output:
node_modules/react-bootstrap/lib/Media.js -> node_modules/react-bootstrap/lib/MediaLeft.js -> node_modules/react-bootstrap/lib/Media.js
Circular dependency detected:
node_modules/react-bootstrap/lib/MediaLeft.js -> node_modules/react-bootstrap/lib/Media.js -> node_modules/react-bootstrap/lib/MediaLeft.js
Circular dependency detected:
node_modules/react-bootstrap/lib/MediaRight.js -> node_modules/react-bootstrap/lib/Media.js -> node_modules/react-bootstrap/lib/MediaRight.js

[taion]

It's intentional, and as used doesn't cause problems.

[zamrq]

https://github.com/mxstbr/react-boilerplate/issues/1268

[grantila]

Going through our projects I missed this one. It's reported in a few other packages, but you are not many who have circular dependencies. Please note that they are never required, and can always be replaced with another design.

The result is that users of this package, and users implicitly depending on this one, will have to adapt and tweak their static analyzers because this package fools them. So it does cause problems, although not necessarily run-time.

I've reported this to unshiftio/url-parse#49 and nodejs/readable-stream#280 too.

Circular dependencies are a bad practice, so please try to get rid of it. It would be pleasant for the community if static analysis (optimizing, tree-shaking, static checks etc) could be used without the hassle of having to manually disable it for certain (potentially implicitly) dependent packages, which in worse case will affect the static analysis negatively.

[jquense]

there is no implicit dependencies they are all completely explicit. not at all sure why static analysis would somehow fail here, its completely valid imports and perfectly analyzable.

[calvinmetcalf]

@grantila I think you are misusing that plugin, note the readme

Circular dependencies are often a necessity in complex software, the presence of a circular dependency doesn't always imply a bug, but in the case where the you believe a bug exists, this module may help find it.

[grantila]

@jquense "valid imports and perfectly analyzable", well.. You're expecting quite a lot of a static analyzer if you believe they can detect run-time issues. They can't, and is why they complain, be it a "false positive" but still. Or please provide an explanation as to how a static analyzer in all situations can detect problematic circular dependencies.

@calvinmetcalf I'm not misusing that plugin, and its author may write whatever he likes about the necessity of circular dependencies, but it doesn't make it any more true.

File-based circular dependencies is unfortunately possible in node/javascript, although it is never necessary. It can always be replaced by another design.

I've added an exception to this package, because it broke my static analyzer (and I don't blame the analyzer), so for me it's fine. You should still consider fixing this and never writing file-based circular dependencies again. It may bite you, regardless of how smart you are.

In complex applications using dynamic loading (like webpack's chunks), the exact load order is non-deterministic and may depend on network/cache/timing. But it's the load order that defines whether a circular dependency will work fine or not, making the code non-deterministically fail. This is why forbidding file-based circular dependency is a good thing, and helps you avoid these situations. When doing so, your package is in the way, just so you know.

I'm not saying re-open, I'm saying, please consider it. People will be upset to see it, and they will rightfully question your choice to implement a file-level circular dependency when you never had to. Cheers!

[jquense]

People will be upset to see it, and they will rightfully question your choice to implement a file-level circular dependency

As noted the existence of a circular dependency is not itself a bug. and if folks insist on being "upset" by correct, non buggy code. thats really on them, not gonna lose sleep over it.

[taion]

The source code is written with ES modules anyway, so the exported thing is actually a binding, and is perfectly statically analyzable – in fact moreso than with CJS.

The "static analysis" tool you mention in the other issues seems to just be looking for circular dependencies. That's not really "breaking", it's the tool doing something irrelevant in this context.