search

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

Separation of Concerns - Code refined #39

Closed mamboer closed 9 years ago

mamboer commented 9 years ago
  1. Rewrote and refined all the codes
  2. Upgraded lwip to version 0.0.6 to support GIF images.
  3. More NodeJS friendly callback API. Using function(err,data) instead of function(data,err) blah blah. Hope that you can merge my codes, thanks.
arvindr21 commented 9 years ago

Thanks @mamboer for this awesome contribution. I was planning to do something similar. Let me pull this code and test it out. I will get back to you as soon as I can.

arvindr21 commented 9 years ago

@mycaule your comments.

mycaule commented 9 years ago

@mamboer I think you should make incremental pull request. You did changes that do not respect the original intent of blueimp, like modifying the FileInfo object. Since the unit tests aren't complete, there is no way to verify that it will not break other users projects.

You Pull Request looks more like a hack to solve the problems you're facing in your project.

The part about separating concerns seems ok though, but it needs to me more structured in my opinon.

JSHinting the code is also needed.

mamboer commented 9 years ago

@mycaule Thanks for your code reviews. My PR did make many breaking changes, and it will absolutely break other users' projects if they do not make any adjustments.

IMHO, backwards compatibility should be solved via releases, we can release v1 in certain phase for that phase's users, then keep moving on a new milestone or cool release , just like jQuery v1 vs v2.

About the FileInfo object, i just added some properties like width and height, of which may be useful in the client side. :)

Anyway, i respect your opinions, cheers on open sources.

mycaule commented 9 years ago

If you are sure, and have done all the basic end to end tests, I am ok with accepting your PR. It's not like this package is as critical as a nuclear plant either.

Fixes based on my remarks and code cleaning with JSHint can be done afterwards.

arvindr21 commented 9 years ago

@mamboer @mycaule I think the PR looks fine. Couple of things

  1. Please make the version as 0.4.0 in package.json and update the readme.md file with clear docs about the change in code structure.
  2. Also please mention a migration document (if you think would be needed to move from 0.3.x to 0.4.0)
  3. Can you please remove latest as dependencies version and add the exact version? This will make sure that our app will not break if the module we are referring changes.

Thanks.

mamboer commented 9 years ago

@arvindr21

Okay, i will do these things later today, along with the code cleaning using JSHint mentioned by @mycaule .

THanks

arvindr21 commented 9 years ago

@mamboer can you please update the 3 things I requested for, so I can merge this PR and release a new version?

mamboer commented 9 years ago

@arvindr21 hi, i've updated that 3 things you requested for, JSHint passed too. Pls be sure to test on a real AWS ENV before making a new release, AWS is not stable in my country and i don't have condition to test it out.

Thanks.

arvindr21 commented 9 years ago

Thanks @mamboer

@mycaule Let me know if you want to make any changes before I push this as 0.4.0 to NPM repo.