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

Already on GitHub? Sign in to your account

consider visionmedia/node-querystring (qs in npm) for parsing #4

Closed
tj opened this Issue Nov 4, 2011 · 24 comments

Comments

Projects
None yet
3 participants
Contributor

tj commented Nov 4, 2011

tough call, but it would be nice to have a drop-in replacement for bodyParser so parted can handle both in the same manner

Owner

chjj commented Nov 4, 2011

Yeah, the querystring parser has gotten the least love out of the three. It is my goal to have parted more or less be a streaming version of the connect body parser. I'll give it a shot soon I think.

Contributor

tj commented Nov 4, 2011

yeah understandable, I just question if it's really worth streaming those bodies, especially since they're buffered in the middleware anyway. Just like working with streaming JSON is (usually) relatively useless

Owner

chjj commented Nov 4, 2011

Ah yeah, it's something I've thought about quite a bit. It depends on what the JSON looks like I guess. A possible benefit would be slightly less memory usage, but there's probably a certain threshold to hit before there is any gain, and I don't know how often people are passing around >100kb of json. The json parser does let people to hook into events for each value instead of using middleware, but you're right, I don't know how useful it is when it's just used as middleware. Despite all that, I do like my json parser, I think I'd want to find a place to put it before I got rid of it in parted.

parted was originally just a multipart parser. I wrote the others because I could, and because I wanted a unified interface for a body parser, where I didn't have to worry about two different middlewares messing with each other by both setting req.body.

Contributor

tj commented Nov 4, 2011

maybe those should be options too, buffer: true would just do what bodyParser does without clashing with it. yeah that's exactly it, you can DoS someone easier with bodyParser but people should limit those anyway ideally.

I agree that's a nice thing, a lot of people are confused with connect-form / using formidable (or parted streaming i suppose) directly so it's nice to have but IMO the nesting thing is necessary

Owner

chjj commented Nov 4, 2011

Yeah, I'm thinking maybe buffer should be the default, and have the streaming json and encoded parser load lazily depending, or maybe drop them, not sure. Full compatibility with the connect bodyparser is nice, and buffering seems more practical, and generally faster. I'll create a branch for this and mess around some.

Contributor

tj commented Nov 4, 2011

awesome sounds good, I know this will help people a ton

Owner

chjj commented Nov 7, 2011

https://github.com/chjj/parted/compare/no_stream
https://github.com/chjj/parted/compare/gutted

The first defaults to non-streaming parsers, the other removes the streaming parsers altogether. It does seem cleaner, less code, less to maintain. I'm considering going that route.

chjj I would really like this it would be huge.

Owner

chjj commented Nov 10, 2011

b5a282f

I'll push it to npm in a little bit.

I just installed the latest 0.8 and I do not see a difference in the parsing. Do I need to change a config setting or something?

Thanks

Owner

chjj commented Nov 10, 2011

Do I need to change a config setting or something?

No, it should just work. Make sure qs is installed. It will fallback to the regular node module if you don't have qs.

I have qs installed. In bodyParser I get this:

{ _csrf: 'FsUImtPfWn2zi2sZ9D6TI17P',
  'account: {
    company_name: 'Data',
    broker: 'Data',
    address: 'Data',
  },
  user: {
    city: 'Data',
    state: 'Data',
    zip: 'Data'
  }
  'user[state]': 'WA',
  'user[zip]': '99019',
  commit: 'SUBMIT >>' }

with parted I get this:

{ _csrf: 'FsUImtPfWn2zi2sZ9D6TI17P',
  'account[company_name]': 'Data',
  'account[broker]': 'Data',
  'account[address]': 'Data',
  'user[city]': 'Data',
  'user[state]': 'Data',
  'user[zip]': 'Data',
  commit: 'SUBMIT >>' }
Owner

chjj commented Nov 10, 2011

Are you sure? I did a fresh npm install just to check, and it works fine when I have fields named: test, user[name], and user[pass].

Output:

{ test: 'asdasd',
  user: { name: 'adasd', pass: 'asdasd' },
  send: 'Submit' }

Ok I will check again.
On Nov 10, 2011 1:32 PM, "Christopher Jeffrey" <
reply@reply.github.com>
wrote:

Are you sure? I did a fresh npm install just to check, and it works fine
when I have fields named: test, user[name], and user[pass].

Output:

{ test: 'asdasd',
user: { name: 'adasd', pass: 'asdasd' },
send: 'Submit' }


Reply to this email directly or view it on GitHub:
#4 (comment)

I did a fresh npm install and am still having the problems. Not sure what it could be.

Owner

chjj commented Nov 10, 2011

Do you see qs in the parted/node_modules directory? If I had to guess, it's that qs isn't getting resolved correctly, and parted is falling back to node's querystring module. If you comment out the try/catch at the beginning of lib/index.js, you'll be able to see whether qs is found.

Ok so the problem is this is a multipart form. If I do this on a normal form it works. Can I not get the same parsing on a multipart form?

Thank you

Owner

chjj commented Nov 11, 2011

No, it won't work on multipart. The only thing that has changed in anyway meaningful for the user is the querystring parser.

That's a good idea though. I want to look into implementing it.

Owner

chjj commented Nov 11, 2011

beaffe7

Won't be pushing that to npm right away, but it seems to work and I like the consistency.

Awesome! I don't see any problems. This is great. Thanks

Owner

chjj commented Nov 17, 2011

Added the streaming parsers back, but they sit behind a stream option. The streaming encoded parser also handles nested fields now.

@chjj chjj closed this Nov 17, 2011

Contributor

tj commented Nov 17, 2011

does it support all the same syntax? images[]=foo&images[]=bar etc?

Owner

chjj commented Nov 17, 2011

Yeah, it should. One thing that was strange behavior-wise was something like: a[][]=1&a[][]=2. Is that supposed to be { a: [ [1], [2] ] } or { a: [ [1, 2] ] }? I could get it either way, but qs doesn't seem to like fields like that. I doubt anyone is doing something like that though.

Contributor

tj commented Nov 17, 2011

who knows haha

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