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 ready event when using config.mongo #219

Closed
wants to merge 2 commits into from
Closed

Conversation

loris
Copy link
Member

@loris loris commented Oct 27, 2015

config.mongo is doing synchronous work, but added ready event for being consistent with config.db behavior (#213 (comment))

@jdiamond
Copy link
Contributor

Testing this PR. Did you mean to add the "failCount" commit? Doesn't seem related to the ready event feature.

@loris
Copy link
Member Author

loris commented Nov 10, 2015

No actually, it was meant for another PR but both ended up on my fork
master branch. I will fix that as soon as I get a computer.

@jdiamond
Copy link
Contributor

So this fix doesn't really work because it emits the ready event before the Agenda constructor has a chance to return. Since I can't call .on('ready') until the constructor returns, I miss it.

I think moving the emitting of the ready event to db_init() (after the indexes are created) might be better so that it only has to be in one spot. As it is now, it's possible to not pass in a config object to the constructor and use the database() or mongo() methods to set those options. They end up calling db_init(), but don't emit the ready event.

@jdiamond
Copy link
Contributor

I took a crack at this here:

https://github.com/jdiamond/agenda/tree/ready-event

Can submit a PR if you think it's a good approach.

@loris
Copy link
Member Author

loris commented Nov 12, 2015

@jdiamond You right, this is the best approach, I will close this PR and test yours on my setup.
Thanks!

@loris loris closed this Nov 12, 2015
@Pyrolistical
Copy link

is there a workaround before this is fixed?

@rschmukler
Copy link
Collaborator

@jdiamond please submit a PR upstream and I'll merge in!

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

4 participants