search

seanemmer / mongoose-seed

Seed data population for Mongoose
MIT License
52 stars 33 forks source link

Callback invoked only when all entries have been processed #6

Closed nessche closed 8 years ago

nessche commented 8 years ago

I have noticed that the populateModels callback was invoked without waiting for the entries being added to the database. This was a problem when running seeder from a script that ends immediately after populateModels, as not all the data may have had time to be written into the DB. So I moved the invocation inside the forEach with a check that make sure it is only called when all the entries have been processed.

alanhr commented 8 years ago

Why not use promise ?

nessche commented 8 years ago

@alanhr, yes you could also use promises, but then I would rewrite the plugin completely to use promises in all methods. Also I would use the promise-version of all underlying mongoose methods, so that e.g. populateModels would just need to build an array of promises and then return Promise.all(arrayOfPromises) rather than keeping track of the number of processed entries.

But I understand @seanemmer approach to use callbacks to be closer to the original node/mongoose paradigm (so that the module does not imply to much about the rest of the environment). This PR is simply a quick fix (took me less than writing this comment) for a problem I faced (and btw I use Bluebird's promisifyAll(require'mongoose-seed') so I get promise-enabled methods).

seanemmer commented 8 years ago

Thanks for flagging this @nessche. I'd like to use the async.each method (see line 93) instead of a counter variable to keep the code consistent. I should have time to implement this myself later but feel free to submit another PR and I'll merge it!

nessche commented 8 years ago

Ok changed the code to use async.eachOf. Please note that I never propagate the possible error from Model.create because I need every document to be processed and I can lookup the errors visually in the console (I use this from a script, not in a constantly running server). Feel free to change it, if you want to be able to propagate the errors to the populateModels callback.

alanhr commented 8 years ago

I like this lib, but we need implement test with mocha

seanemmer commented 8 years ago

Looks good, thanks @nessche.

And yes, Alan, we do. This is something I can look into over the weekend but feel free to contribute in this regard as well.

alanhr commented 8 years ago

Ok, today I going to start this feature