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

I should depend on raw-body #2

Closed
Raynos opened this issue Nov 26, 2013 · 15 comments
Closed

I should depend on raw-body #2

Raynos opened this issue Nov 26, 2013 · 15 comments

Comments

@Raynos
Copy link
Owner

Raynos commented Nov 26, 2013

@jonathanong authored raw-body which has very similar concerns

Either I should depend on it or maybe this module should just be merged with it.

@jonathanong
Copy link

probably depend - raw-body doesn't actually parse the string. i was thinking about making a body-parser module, but i guess this module is what that would be. people in connect/express just want too many options, it's pretty annoying.

i'm not sure what's the point of using the string decoder vs. buffer.toString() though. only issue i can think about that is getting the correct charset from the request body if it isn't utf8.

also, not including string parser is easier for me since then i can implement the string parser however i want. see: https://github.com/koajs/body-parser/blob/master/index.js

@Raynos
Copy link
Owner Author

Raynos commented Nov 26, 2013

@jonathanong So I don't know whether string decoder of Buffer.concat(buffers) is better. should have a bench mark.

I copied @izs code from npm-www which uses StringDecoder which is used to handle two bytes UTF8 properly since body += String(chunk) is wrong if chunk has a trailing byte.

@Raynos
Copy link
Owner Author

Raynos commented Nov 26, 2013

@jonathanong btw I'm going through express & connect and rewriting them using small modules. If you have any modules to recommend using you should add them to the wiki page !

@jonathanong
Copy link

yeah, i don't know which one is faster, but i prefer fewer lines of code. i doubt any performance differences are significant, though.

@jonathanong
Copy link

sick! i actually wanted to do that, but unfortunately a lot of the middleware rely on connect patches like res.on('header').

most of the remaining connect issues are sessions, but sessions right now are such a mess that i don't want to touch them. for koa, tj's been thinking about just making tests for a session "spec" which implementors can use instead of relying on connect.session(). it should be easier to make better session middleware that way. let me know if you're interesting in doing that for connect/express.

@Raynos
Copy link
Owner Author

Raynos commented Nov 26, 2013

@jonathanong look at generic-session & redsess & level-session. that's what I use, I like them.

but of course it doesn't have that crazy asynchronously load session for each req before route and asynchronously save session for each res.end()

@isaacs
Copy link

isaacs commented Nov 26, 2013

Buffer.concat is not better unless for some reason you already HAVE a list of buffers. If you're collecting a stream into a string, then StringDecoder is better in both time and space.

@Raynos
Copy link
Owner Author

Raynos commented Nov 26, 2013

@isaacs I broke out your npm-www things, should I PR them back into npm-www ?

@jonathanong
Copy link

is the impact significant?

also, why even bother using string decoder then? why not just do req.setEncoding()?

@Raynos
Copy link
Owner Author

Raynos commented Nov 26, 2013

@jonathanong req.setEncoding will set the encoding on the source namely the req. Using stringDecoder in body is setting the encoding on the destination, namely the streaming body parser.

If you pipe the req to multiple location then maybe not all locations want the data chunks to be UTF8

@jonathanong
Copy link

oh, that's true. there was a bug where someone decided to set the encoding for no reason.

@jonathanong
Copy link

actually, that wouldn't matter. you assume the data is in buffers as well.

either way, i'll add encoding support to raw-body.

@Raynos
Copy link
Owner Author

Raynos commented Nov 26, 2013

@jonathanong I did a code review of raw-body and added some issues! Once those are handled I'll update this to depend on raw-body instead.

@Raynos
Copy link
Owner Author

Raynos commented Nov 27, 2013

I updated the code to depend on Raynos/raw-body

Waiting for @jonathanong to publish a version of raw-body.

Added @jonathanong as a collaborator aswell. You can make alterations. If your not sure about stuff make PRs and I'll review it.

@Raynos
Copy link
Owner Author

Raynos commented Nov 27, 2013

Now depends on raw-body form npm.

Published as v4.0.2

@Raynos Raynos closed this as completed Nov 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants