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

Code cleanup #38

Closed arvindr21 closed 9 years ago

arvindr21 commented 9 years ago

@mycaule I am planning to push the refactored code to NPM soon. Any thoughts?

mycaule commented 9 years ago

You should check the package with the new version of node v0.12 as well. Same with io.js ? Just edit the .travis.yml for that.

Some packages in my node_modules of my projects broke when migrating to node 0.12.

I still think that having aws and a c++ compiler as a dependency isn't that great. I didn't have so much time lately to do the refactor I promised. Sorry for that.

arvindr21 commented 9 years ago

Not an issue. I want to spend time and clean up the code. You have any design in mind to add additional packages like aws on demand? I can take a look.

mycaule commented 9 years ago

I initially wanted to copy the design of winston and their transports system.

I think it could be a good idea to write an aws-s3-transport.js which requires the aws package and then explicitly ask the user to add aws in their package.json if they want to use the service.

Big packages like [passport] have different plugins with different NPM repos. I think this project is not big enough to maintain several repos though.

arvindr21 commented 9 years ago

True makes sense. If we need to implement a module like winston, we need to wrap aws api as mentioned and maintain it. Let me see how to figure this out.

My only concern is this, as of today, there are 10,000 downloads in the last month. So what ever we do for 0.4.0 should not break the existing apps and at the same time should not involve much efforts on migration form 0.3 to 0.4.

What do you think?

mycaule commented 9 years ago

Well, if you keep the configuration logic (options / storage / type / local vs aws), migration from 0.3 to 0.4 will be as simple as these three commands :

npm install (or update) blueimp-file-upload-expressjs
npm install lwip --save
npm install aws-sdk --save
mycaule commented 9 years ago

Actually one very simple way to cut the package dependency without much code is to use this:

if (options.storage.type === 'aws') {
  var aws = require('aws-sdk');
} else {
  console.log("I am quite happy without Amazon!");
}

Node won't try to require the package in the second case, it is loaded only when necessary during execution.

arvindr21 commented 9 years ago

This makes a lot of sense. I was looking at Broadway and was thinking on those lines but this is even simpler. So I will brach out 0.4.0. Get things done and let you know. We can finalized before merging it.

arvindr21 commented 9 years ago

blueimp-file-upload-expressjs@0.4.0