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

Implement ready event. Fixes #102 #103

Closed
wants to merge 1 commit into from
Closed

Conversation

amasad
Copy link

@amasad amasad commented Feb 24, 2014

A ready event for every .add call seems appropriate to notify the user that we're ready to detect file changes.

@paulmillr
Copy link
Owner

Seems like that complicates chokidar A LOT. Does your current solution work fine for you?

@amasad
Copy link
Author

amasad commented Feb 24, 2014

Nope, it's very flaky, can't find a correct delay time that always works.

I wonder if there is a cleaner way to do this

On Monday, February 24, 2014, Paul Miller notifications@github.com wrote:

Seems like that complicates chokidar A LOT. Does your current solution
work fine for you?

Reply to this email directly or view it on GitHubhttps://github.com//pull/103#issuecomment-35883798
.

@paulmillr
Copy link
Owner

Of course.

@readyFiles = {}

# on add
@readyFiles[newPath] = true

# on unlink
@readyFiles[newPath] = undefined

# when emitting initial event
emit ready unless @readyFiles[path]

@amasad
Copy link
Author

amasad commented Feb 24, 2014

I'm not sure I understand how will this work. But are you open to adding
this in?

On Monday, February 24, 2014, Paul Miller notifications@github.com wrote:

Of course.

@readyFiles = {}

on add

@readyFiles[newPath] = true

on unlink

@readyFiles[newPath] = undefined

when emitting initial event

emit ready unless @readyFiles[path]

Reply to this email directly or view it on GitHubhttps://github.com//pull/103#issuecomment-35895831
.

@paulmillr
Copy link
Owner

yes, the solution I mentioned is a lot simpler and has no perf impact so I will be glad to merge it

@amasad
Copy link
Author

amasad commented Feb 24, 2014

cool, now to try to understand your solution.

emit ready unless @readyFiles[path] how will this work if I want to emit 'ready' for when .add() is called with files and all the files and subdirectory files are added and we're idle waiting for file changes. In that scenario what path are we checking? and why is it unless if I want to make sure they're added?

I must be missing something

@paulmillr
Copy link
Owner

hmm you're right.

OK in this case I suggest you to try out your fork in production. Maybe after some time and tests you will see it's stable and we can think of something then. (not comfortable with merging this big change without any production tests)

@paulmillr paulmillr closed this Feb 25, 2014
@amasad
Copy link
Author

amasad commented Feb 25, 2014

alright, thanks man

@paulmillr
Copy link
Owner

(ready event was merged with 4c20553 in Nov, just like in this PR)

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.

2 participants