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.

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

Circular dependency detected #49

Closed
AnthonyPanchenko opened this issue Jan 4, 2017 · 4 comments
Closed

Circular dependency detected #49

AnthonyPanchenko opened this issue Jan 4, 2017 · 4 comments

Comments

@AnthonyPanchenko
Copy link

AnthonyPanchenko commented Jan 4, 2017

Hi, I have found these circular dependencies in console using plugin "circular-dependency-plugin".

ERROR in Circular dependency detected:
node_modules\url-parse\index.js -> node_modules\url-parse\lolcation.js -> node_modules\url-parse\index.js

ERROR in Circular dependency detected:
node_modules\url-parse\lolcation.js -> node_modules\url-parse\index.js -> node_modules\url-parse\lolcation.js
@lpinca
Copy link
Member

lpinca commented Jan 4, 2017

Yes but it shouldn't cause any harm. We could move lolcation.js contents to index.js to avoid the circular dependency if necessary.

@3rd-Eden
Copy link
Member

3rd-Eden commented Jan 4, 2017

The circular dependency is lazy loaded so this will never be an issue.

@3rd-Eden 3rd-Eden closed this as completed Jan 4, 2017
@grantila
Copy link

"never be an issue" is obviously not really true if the circular dependency requires everybody with this plugin to exclude: /node_modules\/url-parse/. It may affect more static analyzers too.

In a relatively large project I'm working on now (where npm ls | wc -l is 1156), this and readable-stream are the only packages causing this. Surely the code could be written for static analyzers to not be fooled. They can't know it's gonna be fine at run-time.

The fact that the code won't fail run-time doesn't necessarily mean that it's not an issue, if developers (incl. those not directly using this package!) need workarounds for their builds to go through. @lpinca's idea sounds good.

@3rd-Eden
Copy link
Member

3rd-Eden commented May 3, 2017

Published the fix in 1.1.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants