arvindr21 / blueimp-file-upload-expressjs

A simple express module for integrating jQuery File Upload.
http://expressjs-fileupload.cloudno.de/
104 stars 69 forks source link

Error as first parameter in callbacks #23

Closed ganeshten closed 9 years ago

ganeshten commented 9 years ago

example from home page

    router.post('/upload', function(req, res) {
      uploader.post(req, res, function (obj) {
            res.send(JSON.stringify(obj)); 
      });

    });

uploader.post's callback only provides obj it should pass the error as first parameter if there is any error from aws s3 or null if there is not any error.

arvindr21 commented 9 years ago

I know the node convention, but is it acceptable if I pass the error as second argument? Because if I change the first arg to error, it will break the existing code.

Your thoughts?

ganeshten commented 9 years ago

Hi, My opinion is to sticking with community convention will enable easier adaptability of the lib. Yes it will break existing code but fixing this earlier is good so we can avoid future complications :)

arvindr21 commented 9 years ago

Thanks @ganeshten. As of now I think it is best if you can fork this repo and add the requested changes.

If I get a few more requests on this, I will consider the change.

Thanks.

ganeshten commented 9 years ago

@arvindr21 Made a pull request :) url: https://github.com/arvindr21/blueimp-file-upload-expressjs/pull/24

ganeshten commented 9 years ago

@arvindr21 As you said earlier passing it as second argument I've made a another pull request which adds the error as last argument to post callback. url: https://github.com/arvindr21/blueimp-file-upload-expressjs/pull/25

arvindr21 commented 9 years ago

@ganeshten Thanks for the contribution. Would you like to update the docs as well, indicating error is available as second argument.

ganeshten commented 9 years ago

@arvindr21 Pls update the npm repo too :)

ganeshten commented 9 years ago

@arvindr21 Doc update PR url: https://github.com/arvindr21/blueimp-file-upload-expressjs/pull/26