anacronw / multer-s3

multer storage engine for amazon s3
MIT License
660 stars 191 forks source link

Drop requirements for aws-sdk data and dirname #9

Closed simonfan closed 8 years ago

simonfan commented 8 years ago

As aws-sdk auto-loads configurations and credentials from ~/.aws/ directory and the recommended way of loading credentials is by letting the aws-sdk do so, I suggest we drop the requirements for the region, secretAccessKey and accessKeyId.

The dirname option also should not be required, as it is not mandatory to upload the file to a directory (though useful for most cases).

What do you think?

  if (!opts.secretAccessKey) throw new Error('secretAccessKey is required')
  if (!opts.accessKeyId) throw new Error('accessKeyId is required')
  if (!opts.region) throw new Error('region is required')
  if (!opts.dirname) throw new Error('dirname is required')
anacronw commented 8 years ago

I think I agree with the dirname being optional.

On the other hand, aws-sdk might have that behavior, but I don't see a reason why that should be necessarily followed. So as long as the required options are called out explicitly, I'm fine with it :)

LinusU commented 8 years ago

Another way to go would be to let the user provide the aws object. I haven't really thought this thru and I'm not saying it's the way to go, but it could be interesting exploring how the api would look then.

LinusU commented 8 years ago

e.g.

var express = require('express')
var multer = require('multer')
var aws = require('aws-sdk')
var multerS3 = require('multer-s3')

var app = express()
var s3 = new aws.S3({ bucket: 'storage' })
var storage = multerS3(s3, function (req, file, cb) {
  cb(null, 'photos/file-' + Date.now() + '.jpg')
})
var upload = multer({ storage: storage })

app.post('/upload', upload.array('photos', 3), function (req, res, next) {
  res.send('Successfully uploaded!')
})

This way the s3 object can also be used outside of this module, for querying and manipulating the files that where uploaded. It would also drop the dependency of aws-sdk from this module...

LinusU commented 8 years ago

I also think that we should prefer the terminology Key over dirname and filename since that is what amazon calls it. As shown in another issue, calling it dirname has caused some confusion...

anacronw commented 8 years ago

yeah you're right, it should be key instead of dirname. I think I was trying to follow the options on multer.

simonfan commented 8 years ago

I personally like the idea of letting the user provide the s3 object. That way the multer storage engine does not interfere with the rest of the application.

anacronw commented 8 years ago

I'm OK with it either way. Maybe this also gives the user some more control over their configuration.

anacronw commented 8 years ago

Not relevant now b/c it'll just accept s3 options.

Please checkout version 1.5.0