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

Configurable parametersLimit #20

Closed
dougwilson opened this issue Aug 25, 2014 · 6 comments
Closed

Configurable parametersLimit #20

dougwilson opened this issue Aug 25, 2014 · 6 comments
Assignees
Milestone

Comments

@dougwilson
Copy link
Contributor

It would be nice if the second parameter was just an object to specify the options like the separator, parametersLimit, etc. Or maybe copy the interface of https://github.com/joyent/node/blob/v0.10.24/lib/querystring.js#L160 (though I think just a single second options argument is good enough).

Inspiration: expressjs/body-parser#41

@nlf
Copy link
Collaborator

nlf commented Aug 25, 2014

I agree completely, this goes along nicely with #18 as well.

@nlf nlf added this to the 1.3.0 milestone Aug 25, 2014
@nlf nlf self-assigned this Aug 25, 2014
nlf added a commit that referenced this issue Aug 25, 2014
geek added a commit that referenced this issue Aug 25, 2014
make all limits optional, for #18, for #20
@nlf
Copy link
Collaborator

nlf commented Aug 25, 2014

Closed via a5422fc

@nlf nlf closed this as completed Aug 25, 2014
@dougwilson
Copy link
Contributor Author

Holy moley, parsing a flat list of 2000 parameters takes 1710ms on my machine, but < 10ms for querystring module. I don't think allowing over 1000 is even feasible with this module, haha.

@dougwilson
Copy link
Contributor Author

(P.S. yes, I understand this module does more than querystring, just it was the only thing to really compare to :) )

@dougwilson
Copy link
Contributor Author

Looks like for my test, the clone occurring at the top of Utils.merge is causing all the slow down (https://github.com/hapijs/qs/blob/master/lib/utils.js#L52). It doesn't even seem necessary to do in this module? Removing that single clone for me bring down the 2000 parameter parse time to like < 10ms just like querystring.

@nlf
Copy link
Collaborator

nlf commented Aug 27, 2014

Hmm.. I think you're right and we don't need the clone, since we're never calling merge on user input and we don't care if we mutate our own object. I'll open a new issue.

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

No branches or pull requests

2 participants