search

anacronw / multer-s3

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

Remove dirname and pass through S3 return data #12

Closed sabymike closed 8 years ago

sabymike commented 8 years ago

Removes the requirement for dirname and changes the filename property to key. This should resolve issue #9.

In addition, extend the payload passed back to multer and ultimately to the request file property with the data that is provided by the s3.upload call.

LinusU commented 8 years ago

We should note that this is very much a breaking change and should be released under a new major version. I'm all for this change though, :+1:

Maybe add a test with the key function supplied though.

sabymike commented 8 years ago

Updated requested changes including updated test, incremented major version.

LinusU commented 8 years ago

Looks awesome!

I want to give some time to let @badunk look thru this though since the change is rather large. But :+1: from me.

LinusU commented 8 years ago

Also, maybe we should consider baking in the proposed change about passing in an aws instance and dropping the dependency on aws-sdk. On the other hand, maybe that needs some more discussion...

sabymike commented 8 years ago

If so I think that should be a separate pull request. How key is managed and the aws instance are really two separate things. They probably should have been broken out into separate issues.

LinusU commented 8 years ago

Yes, it should absolutely be a separate issue, but the version number should probably not be included here though.

Actually, do you mind removing 7fe6354 (the version bump commit)? I'll do that with npm version major when we are releasing, that will create the correct tags as well. You can do that with git rebase -i master, just remove the line corresponding to that commit.

sabymike commented 8 years ago

Sorry about that, done.

anacronw commented 8 years ago

Looks good to me, not sure what I was thinking when I was calling it filename and dir - maybe I wanted to keep to the same multer options names. I prefer the s3 semantics as well.

Thanks!

LinusU commented 8 years ago

Let's merge, thanks for the awesome work @sabymike!

sabymike commented 8 years ago

Would either of you mind publishing the newest version to npm?

LinusU commented 8 years ago

I don't have access so that must be done by @badunk, I would like to get #13 merged first though. Get all the breaking changes now so we don't have to release 3.0.0 in a week again.

Maybe I'll get some time to look at it tonight (Swedish time)