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

Add option to allow cycles that include an asynchronous dependency

Merged
merged 2 commits into from Apr 27, 2018

Conversation

hedgepigdaniel
Copy link
Contributor

@hedgepigdaniel hedgepigdaniel commented Apr 26, 2018

When using this plugin in conjunction with babel-plugin-universal-import and react-universal-component, warnings are generated for import cycles even if one of the imports in the cycle is an asynchronous import() promise. This happens because of the require.resolveWeak used by babel-plugin-universal-import - it generates a dependency on the module in webpack (with the weak) flag set.

I've confirmed here that even on the server when import() promises do not create separate chunks, the actual execution of the imported module is still asynchronous in the sense that it occurs after the module importing it has finished executing.

So this flag should prevent generating warnings where there is no risk that imports will be undefined as a result of import cycles.

index.js Outdated
@@ -8,6 +8,7 @@ class CircularDependencyPlugin {
this.options = extend({
exclude: new RegExp('$^'),
failOnError: false,
allowAsyncCycles: true,
Copy link
Owner

@aackerman aackerman Apr 26, 2018

Choose a reason for hiding this comment

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

I think the default should be false and consumers of this plugin can opt into ignoring these because they are in fact circular.

Copy link
Contributor Author

@hedgepigdaniel hedgepigdaniel Apr 27, 2018

Choose a reason for hiding this comment

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

Sure, I've changed it to false by default.

Copy link
Owner

@aackerman aackerman Apr 27, 2018

Choose a reason for hiding this comment

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

Thanks I'll get this into a 5.1 version

@aackerman aackerman merged commit 2ea27ea into aackerman:master Apr 27, 2018
1 check passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants