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

hooks and docs to enforce a maximum number of cycles #27

Merged
merged 3 commits into from
Jan 13, 2018

Conversation

turadg
Copy link
Contributor

@turadg turadg commented Dec 4, 2017

Thanks for this plugin. It helped us detect a bunch of cycles. :)

As we knock out cycles, we want to ensure new ones aren't introduced. So we need to error, but can't go full failOnError until we get down to zero.

This PR adds hooks for start and end of the cycle detection, which can be used to set a max cycles policy. It also documents how in the README.


This change is Reviewable

@turadg turadg force-pushed the max-cycles branch 2 times, most recently from 0810463 to 154628f Compare December 4, 2017 19:00
@aackerman
Copy link
Owner

I'm not seeing a strong need for onStartDetecting aside from having symmetry to onEndDetecting even in your example, the variable is initialized before the module and in onStartDetecting.

If you agree I would say go ahead and remove it. Even if you don't, I would like the methods renamed to onStart and onEnd before merging these changes.

@turadg
Copy link
Contributor Author

turadg commented Dec 5, 2017

I'm not seeing a strong need for onStartDetecting aside from having symmetry to onEndDetecting

Is that not reason enough? :)

even in your example, the variable is initialized before the module and in onStartDetecting.

I was thinking it needs to be reset on each start of detection. I use webpack-dev-server and I figured the module level would only run once.

I would like the methods renamed to onStart and onEnd before merging these changes.

Will do.

@turadg
Copy link
Contributor Author

turadg commented Dec 8, 2017

I just rebased off upstream/master

@turadg
Copy link
Contributor Author

turadg commented Dec 13, 2017

@aackerman, are there any other blockers to merging?

@aackerman aackerman merged commit 2bc603e into aackerman:master Jan 13, 2018
@aackerman
Copy link
Owner

Published in version 4.4.0

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