Added `defer` property to multipart() middleware #664

Merged
merged 3 commits into from Oct 2, 2012

Conversation

Projects
None yet
2 participants
@bminer
Contributor

bminer commented Oct 2, 2012

The newly added defer property allows one to defer mulitpart parsing and immediately execute the next middleware. In this case, Formidable's form object is exposed via req.form, allowing middleware to bind to "progress", "error", and/or "end" events.

Fixes issue #638

bminer added some commits Oct 2, 2012

Added `defer` property to multipart() middleware
The newly added `defer` property allows one to defer mulitpart parsing and
immediately execute the next middleware. In this case, Formidable's form
object is exposed via `req.form`, allowing middleware to bind to "progress",
"error", and/or "end" events.
@tj

tj reviewed Oct 2, 2012
View changes

lib/middleware/multipart.js
@@ -99,8 +102,10 @@ exports = module.exports = function(options){
});
form.on('error', function(err){
- err.status = 400;
- next(err);
+ if(!options.defer) {

This comment has been minimized.

Show comment Hide comment
@tj

tj Oct 2, 2012

Member

if ( for these, and we need a test or two, rest looks good thanks!

@tj

tj Oct 2, 2012

Member

if ( for these, and we need a test or two, rest looks good thanks!

This comment has been minimized.

Show comment Hide comment
@bminer

bminer Oct 2, 2012

Contributor

Should be if(!options.defer)... if we don't defer, then it's safe to call next() here. If we do defer, we shouldn't call next() again, right? I've always wondered... what happens if you res.send(...) and then call next(err)???

@bminer

bminer Oct 2, 2012

Contributor

Should be if(!options.defer)... if we don't defer, then it's safe to call next() here. If we do defer, we shouldn't call next() again, right? I've always wondered... what happens if you res.send(...) and then call next(err)???

This comment has been minimized.

Show comment Hide comment
@tj

tj Oct 2, 2012

Member

no i just mean if ( add a space :D haha

@tj

tj Oct 2, 2012

Member

no i just mean if ( add a space :D haha

Minor style changes
Added tests for `defer` option in multipart() middleware
@bminer

This comment has been minimized.

Show comment Hide comment
@bminer

bminer Oct 2, 2012

Contributor

@visionmedia - Made a few of your suggested changes. Please let me know if you need anything else. :)

Contributor

bminer commented Oct 2, 2012

@visionmedia - Made a few of your suggested changes. Please let me know if you need anything else. :)

tj added a commit that referenced this pull request Oct 2, 2012

Merge pull request #664 from bminer/master
Added `defer` property to multipart() middleware

@tj tj merged commit cb80fcd into senchalabs:master Oct 2, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment