Default to disk storage

[charlierudolph]

The readme states

In case you omit the options object, the file will be renamed and uploaded to the temporary directory of the system.

However looking at the code if the options is undefined, it passes in an empty object.
Then if storage and dest are undefined it uses memory storage.

I got the behavior I expected the library to have by default with

var multer = require('multer')
var tmp = require('tmp');
var upload = multer({dest: tmp.dirSync().name});

I would suggest either updating the docs or updating the implementation to be consistent with the docs. I prefer the latter.

[LinusU]

Hmm, this is indeed a problem. I would prefer to fix this in the documentation to avoid breaking anyones code.

I'll push a fix to the documentation, but keep this issue open so that we can discuss how it ideally should work. Than we can see how we can best implement/roll out that.

Ping @jpfluger

[hacksparrow]

How about increment the minor version and default to disk?

[LinusU]

Technically we can't do that: semver

MINOR version when you add functionality in a backwards-compatible manner

But I'm also still not convinced that defaulting to disk is the best alternative. To be honest, I think I'm exactly in the middle between defaulting to memory and disk in temporary location.

Maybe we should do it so that you should always somehow specify one of them. Currently you can do the following two:

multer({ storage: multer.memoryStorage({}) })
multer({ storage: multer.diskStorage({}) })

Maybe there should be a shorthand for doing them, or maybe it's just good as it is...

[charlierudolph]

Can you use diskStorage without passing in a dest?
Looks like it. Can you please add documentation about that?

[LinusU]

Yes you can, I'll update the docs now.

[LinusU]

Landed in 5f0a8c1

[charlierudolph]

Awesome. Thanks @LinusU. One other question. Since all the options to diskStorage are optional how do you feel about making the options object optional?

That way can simply call

multer({ storage: multer.diskStorage() })

[jpfluger]

@charlierudolph Your suggestion makes sense: multer({ storage: multer.diskStorage() })

@LinusU Re. your initial ping, I don't have the perfect 💊 to swallow... but isn't there still an issue with running out of RAM when using in-memory objects? (esp large ones?) If yes, then I'd default to using disk-based storage.

[LinusU]

@charlierudolph I absolutely want to do that, I noticed as well that it didn't work.

@jpfluger Everything should be deallocated properly so it should free the memory after the request is done. It will for a brief time peak at twice the size of the file during the upload while it copies all the data the be sequentially in the memory...

We could also improve the memory performance by allocating the entire file at the start of the stream, provided a Content-Length header is sent for that part.

I think another thing that we should address is to introduce a default max file size, which currently default to Infinity.