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

Consolidate common folders to avoid hitting the FSEvent limit #483

Merged
merged 4 commits into from
May 18, 2016

Conversation

ashaffer
Copy link
Contributor

@ashaffer ashaffer commented May 17, 2016

@es128 Here's the PR for #482. Let me know if you want me to fix anything up or change it.

It does a great job on my project. Goes from 200+ instances to 19-35 (it's somewhat non-deterministic because the way the algorithm works, it depends on the order they accumulate in).

// Decide whether or not we should start a new higher-level
// parent watcher
function couldConsolidate(path) {
var candidates = {};
Copy link
Owner

Choose a reason for hiding this comment

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

Could you conform to our code style here please?

Copy link
Contributor Author

@ashaffer ashaffer May 17, 2016

Choose a reason for hiding this comment

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

Do you want the threshold variable at the top?

Also I noticed candidates is no longer being used. I'll remove that.

EDIT: Also forgot some semicolons right below that. Sorry, I don't use them so I don't even think about it. Adding 'em in.

@ashaffer
Copy link
Contributor Author

ashaffer commented May 17, 2016

Fixed style. I think, let me know if it's still not right.

@coveralls
Copy link

coveralls commented May 17, 2016

Coverage Status

Coverage decreased (-0.06%) to 98.248% when pulling 0630964 on ashaffer:master into 3b6f870 on paulmillr:master.

@coveralls
Copy link

coveralls commented May 17, 2016

Coverage Status

Coverage decreased (-0.1%) to 98.162% when pulling de26ae3 on ashaffer:master into 3b6f870 on paulmillr:master.

}

// Find the parent of a given path
function parentOf (path) {
Copy link
Owner

@paulmillr paulmillr May 18, 2016

Choose a reason for hiding this comment

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

sysPath.dirname??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Sorry, didn't realize that dirname worked for directories parents as well.

@coveralls
Copy link

coveralls commented May 18, 2016

Coverage Status

Coverage decreased (-0.1%) to 98.214% when pulling 7c92a19 on ashaffer:master into 3b6f870 on paulmillr:master.

@paulmillr paulmillr merged commit 2c7fed9 into paulmillr:master May 18, 2016
@ashaffer
Copy link
Contributor Author

Any chance we could get a patch version bump for this so I can start using it in watchify without maintaining a fork?

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.

3 participants