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

Emit a start event when a job begins to process #58

Merged
merged 3 commits into from Feb 24, 2019

Conversation

joelgriffith
Copy link
Contributor

Really appreciate this module and all the work that has gone into it. It's a great example of a simple library that does one thing really well, and is readable. Thanks so much!

This adds a simple start event when a job begins (and exposes that job fn to any listeners). This is useful, in my case, where we append properties to the job so that we can track things like duration in the queue, when it started etc. Adds a test case as well.

@coveralls
Copy link

coveralls commented Feb 24, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 8f08ea7 on joelgriffith:master into 244cb2f on jessetane:master.

@jessetane
Copy link
Owner

Thanks for the patch. Generally in favor but a couple thoughts - mainly, if the job calls back synchronously, I think the success event will fire before the start event, which seems a bit odd.. I wonder if emitting start before the job actually runs works and is ok for you?

Also, in the test, what do you think about moving the event check to the beginning (you could remove the handler with the job) so that we don't need to add the extra call to start() and everything can run in one go?

@joelgriffith
Copy link
Contributor Author

Thanks for the feedback, adjusted!

@joelgriffith joelgriffith changed the title Emit a start even when a job begins to process Emit a start event when a job begins to process Feb 24, 2019
@jessetane jessetane merged commit dccd818 into jessetane:master Feb 24, 2019
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

3 participants