ES6 6.0 / Node v4.2.4+ Peer Review for v6.0.0 #73
Conversation
| @@ -1,8 +1,7 @@ | |||
| language: node_js | |||
|
|
|||
| node_js: | |||
| - "4" | |||
| - "stable" | |||
| - "4.2.4" | |||
There was a problem hiding this comment.
Do you want to run tests only on nodejs 4.2.4?
|
@Jokero - made the changes, need to figure out why tests are failing with some debugging later today. Thanks for the review! |
| result.pages = Math.ceil(data.count / limit) || 1; | ||
| } | ||
| if (typeof callback === 'function') { | ||
| return callback(null, result); |
There was a problem hiding this comment.
I think we should not return result of callback as promise value:
if (typeof callback === 'function') {
callback(null, result);
}
return Promise.resolve(result);
|
@Jokero Check it out, all tests passing. Looks pretty sexy. We can make some code smaller but it seems to be working great now. Thoughts on release as |
| @@ -18,13 +20,13 @@ | |||
|
|
|||
| function paginate(query, options, callback) { | |||
| query = query || {}; | |||
| options = Object.assign({}, paginate.options, options); | |||
| let select = options.select; | |||
| options = options || {}; | |||
There was a problem hiding this comment.
Why did you remove paginate.options?
There was a problem hiding this comment.
@Jokero There was nothing ever sent into paginate.options and that which was wasn't being tested. It could be added if we had it in the docs and tests for it ... feel free to push :)
There was a problem hiding this comment.
paginate.options, limit=0, leanWithId=true were added in 5.0.0, tested and documented
https://github.com/edwardhotchkiss/mongoose-paginate/tree/v5.0.0#set-custom-default-options-for-all-queries
https://github.com/edwardhotchkiss/mongoose-paginate/tree/v5.0.0#zero-limit
|
@Jokero I added back paginate.options, the id field is back. One issue left is the |
|
Also all tests passing except with limit zero. |
|
I use |
|
Yeah limit:0 in MongoDB defaults to all docs as the default behavior. I'll make sure that there is a note of it in the docs. |
|
Pagination is fetching and counting. If we set |
|
@Jokero limit:0 back to original ... 🎱 |
Instead of create an object and then filter by avalable options from object keys, we can use an array and resolve a promise for limit: 0 option
|
Is this due to be merged sometime soon? Reason being is that I want to start adding a feature but I don't want to do it on the old codebase and end up in merge hell :) |
|
Ah that's terrible news. Sorry to hear it. @simison I can have a look at getting this PR finalised, I'm quite time constrained so I don't know how soon I'll be able to do it, but I'll update this ticket as I progress. |
Simple way to handle/create required promises internally
| */ | ||
|
|
||
| mongoose.Promise = global.Promise; |
There was a problem hiding this comment.
I would be wary about this line. Some people like to set mongoose.Promise to their own promise implementation (e.g. bluebird). Overwriting it here would cause them lots of headaches.
If you guys have a few mins! @Jokero @niftylettuce @tsm91 @MatthewRayfield @connected-dalmer @villesau @shime - also please make sure that you are in contributors in
package.json, thanks!