multer icon indicating copy to clipboard operation
multer copied to clipboard

Switch to stream based API

Open LinusU opened this issue 7 years ago β€’ 70 comments

As have been discussed in a few other issues, I thought it would be interested to see what the api would look like if Multer just gave you .stream on the uploaded files. This removes the concept of storage engines completely and you can use any other module that works with streams, instead of multer-specific modules.

I would love some thoughts and feedback on this ❀️

ping @expressjs/multer-collaborators, @wesleytodd

LinusU avatar Sep 29 '16 06:09 LinusU

Love this idea. It is much simpler, more modular, and would solve way more use cases than the current situation (#356 for example). I don't have time now to read all the code, but in theory this has my thumbs up. I will try to look through it more throughly this weekend.

wesleytodd avatar Sep 29 '16 14:09 wesleytodd

My only issue @LinusU is to fix it so that you expose _makeMiddleware again because it's needed in koa-multer here https://github.com/koa-modules/multer/blob/master/index.js#L19. Here was the commit it was removed in https://github.com/expressjs/multer/commit/42a9e3b6376ddd9878f2eb26060713e7479ccb3f.

niftylettuce avatar Oct 01 '16 11:10 niftylettuce

Just released 2.0.0-alpha.1 for you guys to try it out. It's published under the next-tag and thus won't be automatically installed for anyone.

πŸŽ‰

LinusU avatar Oct 01 '16 13:10 LinusU

@niftylettuce I'll look into the koa-issue shortly πŸ‘Œ

LinusU avatar Oct 01 '16 13:10 LinusU

@niftylettuce see koa-modules/multer#11

LinusU avatar Oct 01 '16 14:10 LinusU

You are a rockstar. How do I donate to you?

On Oct 1, 2016 10:02 AM, "Linus UnnebΓ€ck" [email protected] wrote:

@niftylettuce https://github.com/niftylettuce see koa-modules/multer#11 https://github.com/koa-modules/multer/pull/11

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/expressjs/multer/pull/399#issuecomment-250914034, or mute the thread https://github.com/notifications/unsubscribe-auth/AAf7hWB-603_neTY5DRoVIaJ8iV_WiTdks5qvmfagaJpZM4KJnV5 .

niftylettuce avatar Oct 01 '16 18:10 niftylettuce

Also @LinusU - is limits option still enforced?

niftylettuce avatar Oct 02 '16 08:10 niftylettuce

How do I donate to you?

There are others who need it more than I, e.g. https://my.charitywater.org/donate/home

is limits option still enforced?

Yes they are :ok_hand:

LinusU avatar Oct 02 '16 17:10 LinusU

2.0.0-alpha.2 is out with improvements to the error codes. Happy coding :tada:

To try it out, use the following command: npm install --save multer@next

LinusU avatar Oct 02 '16 17:10 LinusU

πŸ‘πŸŽ‰πŸ‘ On Sun, 2 Oct 2016 at 10:44 PM, Linus UnnebΓ€ck [email protected] wrote:

2.0.0-alpha.2 is out with improvements to the error codes. Happy coding πŸŽ‰

To try it out, use the following command: npm install --save multer@next

β€” You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/expressjs/multer/pull/399#issuecomment-250982577, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5_YBKGjMMJ9UAM2cWfbp0LBglL5mHSks5qv-Z_gaJpZM4KJnV5 .

hacksparrow avatar Oct 02 '16 17:10 hacksparrow

It's working great for me! I have a fork of koa-multer with this version added and @LinusU fork fix https://github.com/niftylettuce/koa-multer/tree/lu-fix

niftylettuce avatar Oct 03 '16 01:10 niftylettuce

Solves many problems, thanks!

But I don't understand why no encoding and mimetype properties in a file? I think it's critical.

qwelias avatar Oct 20 '16 13:10 qwelias

Oh nevermind, mime type is usually determined by file's extension, and file encoding would not help at anything. Correct if wrong.

qwelias avatar Oct 20 '16 14:10 qwelias

That is correct, as I have understood it the encoding is only how it's sent over the wire and doesn't matter at all for the end user

LinusU avatar Oct 20 '16 14:10 LinusU

@dougwilson Would you mind giving this a quick look? πŸ’Œ

I'm close to feeling that this is ready for releasing :)

LinusU avatar Dec 22 '16 17:12 LinusU

@LinusU CHANGELOG needs an update.

hacksparrow avatar Dec 23 '16 06:12 hacksparrow

  • [x] changelog updated

LinusU avatar Jan 09 '17 21:01 LinusU

Since it will help people keeping their apps secure, and since a lot of the opened issues are about it, I'm leaning towards including mime-detection with 2.x. This is my current thoughts:

The File object would gain the following fields:

// The detected mime-type, or null if we failed to detect
detectedMimeType: String | null

// The typical file extension for files of the detected type, or null if we failed to
// detect (with leading `.` to match `path.extname`)
detectedFileExtension: String | null

// The mime type reported by the client using the `Content-Type` header, or null if the
// header was absent
clientReportedMimeType: String | null

// The extension of the file uploaded (as reported by `path.extname`)
clientReportedFileExtension: String

Any thoughts on this would be appreciated; anyone is welcome to give feedback! :)

LinusU avatar Jan 09 '17 22:01 LinusU

@LinusU I think this is a great idea, as I imaging almost everyone needs most of this info anyway. I know I do in my apps.

wesleytodd avatar Jan 09 '17 22:01 wesleytodd

Note to self: When releasing this, remember to update this page: http://expressjs.com/en/resources/middleware/multer.html

LinusU avatar Jan 11 '17 23:01 LinusU

Released [email protected] with file type detection support πŸŽ‰

LinusU avatar Feb 14 '17 07:02 LinusU

Released [email protected] with a fix for #455 πŸŽ‰

LinusU avatar Feb 14 '17 20:02 LinusU

Released [email protected] with a new error code that will be given when a client aborts a request. Related to #438

LinusU avatar Feb 18 '17 15:02 LinusU

Fairly stupid question because I'm not terribly familiar with multer other than whats on the docs, but what is the API to consume the stream? I essentially want to proxy the multipart file upload request to another backend and would like to take the stream and pass it along

oshalygin avatar Mar 26 '17 07:03 oshalygin

@oshalygin You can see the docs here: https://nodejs.org/api/stream.html

But most likely you just want to pipe the entire request if you want to maintain the multipart form encoding. In which case you wouldn't need this module. If on the other hand, you want to pipe just one file to the upstream, then you could decode the multipart form with this module, and just write the one file to the upstream.

wesleytodd avatar Mar 26 '17 16:03 wesleytodd

@wesleytodd awesome thanks I'm going to give that a whirl.

oshalygin avatar Mar 26 '17 23:03 oshalygin

@LinusU I like this idea of switching multer API to stream. :)

Few thoughts regarding multer new api implementation: Are you creating a temporary file for each file field and then creating read stream from that file? (https://github.com/expressjs/multer/blob/explore-new-api/lib/read-body.js#L71) If so, couldn't we get the same outcome with current multer using DiskStorage - createReadStream and then remove/unlink created file? (https://github.com/expressjs/multer/blob/explore-new-api/lib/middleware.js#L25) Furthermore, what if someone would prefer to stream/pipe uploaded file through memory instead of a temporary file?

Last one: what are your thoughts about an option to pipe the request's stream file directly? (Without writing anything to a temporary file or memory, although I guess that's not the purpose of this library).

orenklein avatar Apr 03 '17 15:04 orenklein

Hi @orenklein, sorry for the late reply :)

Are you creating a temporary file for each file field and then creating read stream from that file? If so, couldn't we get the same outcome with current multer using DiskStorage - createReadStream and then remove/unlink created file?

That is probably doable, although here it would be done automatically for the user...

Furthermore, what if someone would prefer to stream/pipe uploaded file through memory instead of a temporary file?

My plan is to let this decide itself, so that small files only ends up in memory, and large files on disk. Since the new api completely hides this implementation detail, it could be added at a later date without anything needing to be changed for the user!

Last one: what are your thoughts about an option to pipe the request's stream file directly? (Without writing anything to a temporary file or memory, although I guess that's not the purpose of this library).

I think that this use case is better served by busboy, or by having a lower level api that we in itself build Multer on top of

LinusU avatar Aug 05 '17 13:08 LinusU

@LinusU Can you publish a new package with this?

niftylettuce avatar Nov 25 '17 17:11 niftylettuce

Actually I mean - why don't we do this directly in a new major version of multer?

niftylettuce avatar Nov 25 '17 17:11 niftylettuce