Callback should be always last argument

[villesau]

Hi!

There is convention in node.js, that callback is always the last argument of function. Current implementation of mongoose-paginate does not follow this convention and cause conflicts for example with Bluebird. Even though the options might not be used in all situations, i would always appreciate the conventions so it does not cause uncertainty with developers. It is possible to take length of passed arguments and deduce if there is options passed or not.

[scarletsky]

+1 I think we can put all options together, like:

Model.paginate(query, option, callback);

Users.paginate({}, {
    page: 1,
    limit: 20,
    lean: true,
    sortBy: { username: 'asc' },
    // .......
}, function (err, users) {
    // do some thing ...
})

[edwardhotchkiss]

I'm drunk. Do a fix, and a pull request. @niftylettuce thoughts?

:8ball:

[villesau]

i did a pull request of it: https://github.com/edwardhotchkiss/mongoose-paginate/pull/36 Also added jshint-stuff.

[scarletsky]

@villesau @edwardhotchkiss Why not make the callback to be a regular node function with two arguments(err and results) ?

// before
Model.paginate(query, options, function(error, pageCount, paginatedResults, itemCount) {
    //....
}

// after
Model.paginate(query, options, function(error, results) {
    /*
     * results is a object.
     * {
     *      pageCount: pageCount, 
     *      paginatedResults: paginatedResults,
     *      itemCount: itemCOunt
     * }
     */ 
}

[villesau]

+1 That's actually good practice too. I just wanted to fix the 'nodeback' -problem primarily. Should i add the fix for that in my current pull request, or do i make a new branch?

[scarletsky]

@villesau You should ask @edwardhotchkiss :laughing:

[villesau]

Yeah true that. Btw there is one caveat that these changes will break the backwards compatibility. But i think it is OK to sacrifice the backwards compatibility to make it follow regular conventions though.

[scarletsky]

@villesau I agree. In fact, when I use this plugin for the first time, I feel confused about the API. It is really NOT friendly for developer. So, I think it will be better to make it to be a regular node function.

@edwardhotchkiss what do you think about this?

[niftylettuce]

@scarletsky @villesau Please do a pull request. Also you may want to put a deprecation notice in the code. It is a developers responsibility to ensure they are always staying up to date if they are not using specific versions. We can do a major version release bump for clarity as well in NPM. I would agree that the current API implementation is not developer-friendly. Callbacks should always be last.

[edwardhotchkiss]

Thanks @niftylettuce @scarletsky @villesau

[villesau]

@edwardhotchkiss @niftylettuce There is now pull request of cleaner callback too. The pull request contains also all my previous changes like test fixes and method signature change. #37

[niftylettuce]

Closed per #37, releasing new version to NPM with this in a bit. Will add deprecation notice.

[niftylettuce]

See v3.1.5 and updated Readme documentation here

[MarkCancellieri]

@niftylettuce This seems to be a breaking change. You probably should have updated the major version rather than the minor version according to the semver rules. Don't you agree?

[villesau]

@MarkCancellieri +1. I guess many builds will break because of this update. Is it possible to revert back the minor version bump and change it to major?

[MarkCancellieri]

@villesau I'm still a newbie programmer, so I'm not sure what the best way to handle it is. All I know is that I updated to the new minor version and it broke my app, so I had to revert back to the older version until I change my app.