multipart middleware: Capture 'progress' event #638

Closed
bminer opened this Issue Aug 2, 2012 · 13 comments

Projects

None yet

3 participants

@bminer

multipart middleware: Add the ability to listen for the 'progress' event emitted by Formidable's IncomingForm Object.

Perhaps the entire IncomingForm Object could be exposed via the HTTP Request Object? Perhaps passing a "progress" option into the middleware would register the custom event handler?

http://www.senchalabs.org/connect/multipart.html
https://github.com/felixge/node-formidable

@bminer

Similar issue: #565. @visionmedia - I don't fully understand your reasoning for excluding this feature.

@tj
Sencha Labs member
tj commented Aug 2, 2012

few reasons, but the main one being that you might as well drop down to the formidable level and omit connect.multipart() if you need the events anyway. Also depending on the browsers you support this feature is (relatively) useless since you can report progress from the client, it still has use if you are reporting the progress for other connections though. If we can come up with an API for it that isn't really lame I'll consider it

@bminer

@visionmedia - Ok. The more I think about it... I think I see your point now. Thanks.

@bminer bminer closed this Aug 2, 2012
@tj
Sencha Labs member
tj commented Aug 2, 2012

it's a bit unfortunate since mutlipart() is really handy, we could possibly do something but it wouldn't be very pretty. multipart({ progress: callback }) etc is a little weird

@bminer

multipart({ progress: callback }) is more than a little weird... often times, your progress callback will be specific to your route, so I think that this method is shot. What about just exposing the IncomingForm Object on the Request Object? Something like req.form.on("progress", callback) might be okay... I dunno if form is the appropriate property to use on the Request object, but... definitely a better approach IMHO.

@tj
Sencha Labs member
tj commented Aug 13, 2012

we could add some kind of option and then have req.form or req.on('form' available, but the default IMO should still be the convenient thing we have now

@bminer

@visionmedia - if I understand you correctly, multipart (or even bodyParser) would accept an option to expose the IncomingForm Object as req.form or whatever. By default, the Object would not be exposed. Does that sound correct?

@bminer bminer reopened this Aug 14, 2012
@tj
Sencha Labs member
tj commented Aug 14, 2012

yeah, im not sure what we could call it, but it would next() right away and expose the form via event or req.form like you say. defer: true or something

@bminer

Ah. I was hoping for something more like... multipart would still bind to the IncomingForm's field, file, and end events. Then, it would expose req.form and call next(). Obviously, the end event handler would not call next(). The option could be called deferMultipartParse or something, like you mentioned.

Then, in the route, you bind your progress handler, an additional end and error event handler, and invoke req.form.parse(req);.

If you just call next() right away and expose req.form, then like you mentioned earlier, you might as well go to the Formidable level.

@bminer

I guess... what I'm trying to say is... the default functionality of multipart to load stuff into req.body and req.files is really helpful. Most of the time, the only extension is to add an event handler to Formidable's progress event.

@tj
Sencha Labs member
tj commented Aug 14, 2012

that's what I'm saying :p multipart() would still do the nice things it does, but not wait until things are done so you can tack on listeners. I mean realistically you can ignore formidable all together and just add middleware before multipart(), check the Content-Length and do your own progress reporting easily, but it would still be nice to have access to the individual parts

@bminer

Right, okay. Want me to give this a go and submit a pull request? If so, any preference for which branch?

@tj
Sencha Labs member
tj commented Aug 14, 2012

sure if you want to, master branch would be good

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