Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to stream based API #399

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

Switch to stream based API #399

wants to merge 54 commits into from

Conversation

@LinusU
Copy link
Member

@LinusU LinusU commented Sep 29, 2016

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

@wesleytodd
Copy link
Member

@wesleytodd wesleytodd commented Sep 29, 2016

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.

@LinusU LinusU changed the title [WIP] Switch to stream based API Switch to stream based API Oct 1, 2016
@niftylettuce
Copy link
Member

@niftylettuce niftylettuce commented Oct 1, 2016

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 42a9e3b.

@LinusU
Copy link
Member Author

@LinusU LinusU commented Oct 1, 2016

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
Copy link
Member Author

@LinusU LinusU commented Oct 1, 2016

@niftylettuce I'll look into the koa-issue shortly 👌

@LinusU
Copy link
Member Author

@LinusU LinusU commented Oct 1, 2016

@niftylettuce
Copy link
Member

@niftylettuce niftylettuce commented Oct 1, 2016

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

On Oct 1, 2016 10:02 AM, "Linus Unnebäck" notifications@github.com wrote:

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#399 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAf7hWB-603_neTY5DRoVIaJ8iV_WiTdks5qvmfagaJpZM4KJnV5
.

@niftylettuce
Copy link
Member

@niftylettuce niftylettuce commented Oct 2, 2016

Also @LinusU - is limits option still enforced?

@LinusU
Copy link
Member Author

@LinusU LinusU commented Oct 2, 2016

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 👌

@LinusU
Copy link
Member Author

@LinusU LinusU commented Oct 2, 2016

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

@hacksparrow
Copy link
Member

@hacksparrow hacksparrow commented Oct 2, 2016

👍🎉👍
On Sun, 2 Oct 2016 at 10:44 PM, Linus Unnebäck notifications@github.com
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
#399 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA5_YBKGjMMJ9UAM2cWfbp0LBglL5mHSks5qv-Z_gaJpZM4KJnV5
.

@niftylettuce
Copy link
Member

@niftylettuce niftylettuce commented Oct 3, 2016

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

@LinusU LinusU mentioned this pull request Oct 20, 2016
@qwelias
Copy link

@qwelias qwelias commented Oct 20, 2016

Solves many problems, thanks!

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

@qwelias
Copy link

@qwelias qwelias commented Oct 20, 2016

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

@LinusU
Copy link
Member Author

@LinusU LinusU commented Oct 20, 2016

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

var stream = fs.createReadStream(file.path)

stream.on('end', function () {
fs.unlink(file.path, function () {})
Copy link
Member Author

@LinusU LinusU Dec 22, 2016

Actually, we can unlink this file as soon as it has been opened, that way the file wills still be removed even if it isn't consumed. More info: http://stackoverflow.com/a/3181726/148072

index.js Outdated
@@ -12,29 +14,29 @@ function _middleware (limits, fields, fileStrategy) {

class Multer {
constructor (options) {
this.limits = options.limits
this[kLimits] = options.limits

out of curiosity, what's the benefit of using a Symbol here?

Copy link
Member Author

@LinusU LinusU Mar 16, 2020

This was made so that the limits can't be seen/modified from the outside.

I don't really have anything against that, but if it's possible it should be documented, and since it wasn't possible before I don't think that there is a use case for it.

If we leave it in but don't document, then my fear is that we might break it in a future update without meaning to ☺️

makes complete sense! thank you

@ryhinchey
Copy link

@ryhinchey ryhinchey commented Mar 15, 2020

Also this should now close #851

@LinusU
Copy link
Member Author

@LinusU LinusU commented Mar 16, 2020

@bwgjoseph great feedback, I'll add that!

edit: does this look good? Feedback from everyone welcome!

Screenshot 2020-03-16 at 11 30 10

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
`fields` | Max number of fields per request | `1000`
`fileSize` | Max number of bytes per file | `'8MB'`
`files` | Max number of files per request | `10`
`headerPairs` | Max number of header key-value pairs | `2000` (same as Node's http)
Copy link
Member

@gireeshpunathil gireeshpunathil Mar 21, 2020

IIRC, one of the outstanding issue with multer was about better observability of filtered out uploads - for example, is there any inherent mechanism / object that stores the state of failed uploads? Right now the absence of a user callback invocation means to indicate a failed upload, but absence of a callback invocation cannot be intercepted?

@bwgjoseph
Copy link

@bwgjoseph bwgjoseph commented Mar 25, 2020

H-i, was wondering how do you detect the mimeType that is return in detectedMimeType? I tested a couple of files and it all return as null

Like .md, .js, .json and even .txt file.

@LinusU
Copy link
Member Author

@LinusU LinusU commented Mar 25, 2020

@bwgjoseph we are using the file-type package

Unfortunately, detecting the file type of text based formats are very hard, since there isn't much to go on.

e.g. a .txt file can contain any text, so we cannot really "detect" it. An .md Markdown file is also really hard, and would certainly have many false positives (in fact, I think that all text files are valid Markdown files?)

@bwgjoseph
Copy link

@bwgjoseph bwgjoseph commented Mar 26, 2020

@LinusU, apologies as I didn't read much beforehand. My example isn't really great, I see that you're using file-type so I can just dive into that to see what they provide and what not. But thanks!

@LinusU
Copy link
Member Author

@LinusU LinusU commented Apr 14, 2020

Hi everyone, sorry for not having released a final version yet! I just realised that there is one last semi-breaking change that I want out. I'll try and do this tonight and push a new RC.

I want to go thru and improve all the error messages, and give them proper attributes for all relevant data. This seems to be a huge source of confusing for some people, especially the "unexpected field" one, so I want to improve on that!


busboy.on('field', function (fieldname, value, fieldnameTruncated, valueTruncated) {
if (fieldnameTruncated) return reject(new MulterError('LIMIT_FIELD_KEY'))
if (valueTruncated) return reject(new MulterError('LIMIT_FIELD_VALUE', fieldname))
Copy link

@noonii noonii Apr 24, 2020

When an error is thrown from field event, where are the uploaded files removed? Often files are parsed before fields.

EDIT: windows 10, I see the file hanging in the temp directory.

EDIT2: const target = temp.createWriteStream() executes even after fileStream.on('limit') is triggered. The temp file hangs.

@bwgjoseph
Copy link

@bwgjoseph bwgjoseph commented Jul 12, 2020

Hi @LinusU, any further update to this release? Thanks!

@prateekkathal
Copy link

@prateekkathal prateekkathal commented Dec 7, 2020

Waiting on this 😞

LinusU added 2 commits Jul 26, 2021
Migration Guide:

This relases changes the package from a Common JS module to an EcmaScript module, and drops support for older versions of Node.

- The minimum version of Node.js supported is now: `12.20.0`, `14.13.1`, and `16.0.0`
- The package must now be imported using the native `import` syntax instead of with `require`
@LinusU
Copy link
Member Author

@LinusU LinusU commented Jul 29, 2021

@bwgjoseph, @prateekkathal I've published a new RC that fixes compatibility with Node.js 16 🚀

The majority of our dependencies (and the ecosystem at large) is moving to ESM over CommonJS. In order to be able to import those packages (e.g. fs-temp 2.0 which fixes Node.js 16 compatibility) we also need to be an ESM package.

Potentially we could work around this if this is too much of a disruption for people, but I personally feel that this is the way forward for the entire Node.js ecosystem 🚀

Here is the changelog for 2.0.0-rc.3:

Migration Guide:

This relases changes the package from a Common JS module to an EcmaScript module, and drops support for older versions of Node.

  • The minimum version of Node.js supported is now: 12.20.0, 14.13.1, and 16.0.0
  • The package must now be imported using the native import syntax instead of with require

Any and all feedback is much welcomed!

@jonchurch
Copy link
Member

@jonchurch jonchurch commented Jul 30, 2021

It's not yet stable in Node, but WHATWG streams seem like they'd be a nice addition here eventually. That might be Multer 3.0, though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet