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

Added --interval parameter to specify the polling interval for directory/file changes. #84

Closed
wants to merge 2 commits into from

Conversation

ashleydavis
Copy link
Contributor

No description provided.

@finnp
Copy link
Collaborator

finnp commented Dec 18, 2015

I like this. Can you please rebase and also add this to the Readme?

@ashleydavis
Copy link
Contributor Author

Sorry. I did make an update to the readme! For some reason it wasn't committed.

@ashleydavis
Copy link
Contributor Author

I'm on holiday for 2 weeks now. I'll have to commit when I'm back in the office.

@ashleydavis
Copy link
Contributor Author

Readme is updated.

@levithomason
Copy link
Collaborator

Before this is merged, can we set a faster default? The default watchFile interval is 5007 ms.

This is why watch has been noted as being so slow. If you simply set a default interval of 100ms the performance is dramatically increased.

@@ -24,6 +24,9 @@ if (argLen > 1) {
}

var waitTime = Number(argv.wait || argv.w)
if (argv.interval || argv.i) {
watchTreeOpts.interval = Number(argv.interval || argv.i) * 1000.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested default of 0.1s (node default is 5.007s!):

watchTreeOpts.interval = Number(argv.interval || argv.i || 0.1) * 1000.0;

@levithomason
Copy link
Collaborator

Is this project maintained / will this PR be merged soon? I am willing to duplicate the branch and update it if so. Would love to have this improvement added.

cc @ashleydavis @finnp

@levithomason
Copy link
Collaborator

There are other libs that are super at file watching in terms of performance and cleanup. nodemon being one of them. Looking at how they pulled it off, they used paulmillr's chokidar instead of fs.watch/watchFile.

It may be worth considering a move to chokidar for performance and stability increases. Again, something I'm totally willing to help with. I really love watch, let's get it moving again! :trollface:

@ashleydavis
Copy link
Contributor Author

Feel free to fork and update my repo. I'll merge your changes. I have no idea if/when this PR will be merged back to main repo.

@levithomason
Copy link
Collaborator

Thanks @ashleydavis

@mikeal do you know the status of this repo?

@levithomason
Copy link
Collaborator

@finnp @mikeal any way we can get more contributors on this project? There are many valid issues and great PRs. I'd love to help review and merge these.

@mikeal
Copy link
Owner

mikeal commented Mar 24, 2016

I am in complete agreement, I'll add anyone with a notable change as a committer, just give me a list :)

@levithomason
Copy link
Collaborator

Starter list:

  1. levithomason

😄

EDIT

What i'm proposing to do is review issues, review PRs, ensure tests and sanity, and run any thing by you before merging. I'll thoroughly annotate stuff for you as well.

Note, I'm a trusted contributor to https://github.com/shelljs and you can see my other work on my profile. Most of it is in orgs I belong to.

@jordanluyke
Copy link

+1

@levithomason
Copy link
Collaborator

levithomason commented Apr 20, 2016

...I'll add anyone with a notable change as a committer...

@mikeal not exactly sure what is meant by this.

I use watch all over the place but the 5s default watch time is killing me. I'm fully willing and able to address the stale issues and PRs on this repo if you're willing to add me.

If this project is not going to add collaborators and also not address issues/pulls please let us know so we can move on. Would much rather keep things here if possible.

cc @finnp

@levithomason
Copy link
Collaborator

Woop! This can be closed now, thanks to @mikeal for merging #97.

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.

5 participants