search

tj / node-querystring

querystring parser for node and the browser - supporting nesting (used by Express, Connect, etc)
MIT License
455 stars 66 forks source link

Please, this will save foreign chars posts #19

Closed tcha-tcho closed 13 years ago

tcha-tcho commented 13 years ago

URI not properly coded broke the process curl -H "Content-Type: application/x-www-form-urlencoded" -d "{%:%}" -X POST "http://localhost:3000"

Please let us to deal with this problem at the main code later... We could back to this problem later with some safe function.

Thanks in advance :)

Tcha-Tcho

tj commented 13 years ago

test or two please :D

tcha-tcho commented 13 years ago

Hi,

The only thing I've notice was a little (I mean real little) decrease on benchs. But this can vary a lot and its not significantly notice on all benchmarks done. This was the maximum difference between the two:

with try times: 100000 simple: 1764ms nested: 5260ms

without try times: 100000 simple: 1421ms nested: 4444ms

I did run the two tests, Im not familiar with should (Im user of rspec) but I assume that he passed because he gave me no messages with or without the changes.

Node Js have support for a lot of encoding but this part of the code broke not just the hole paypal system but all windows based messaging services. Heroku and others managed servers have node_modules on the cloud and we cannot fixed that on server. Bundle everything will increase the slug sizes to the limit.

We really appreciate if you accept this pull.

Thank you very much Mr. Holowaychuk

Best Regards from Brazil! Tcha-Tcho

tj commented 13 years ago

4675d70

tcha-tcho commented 13 years ago

Totally awesome! thanks Mr. Holowaychuk. I did try to put a log on catch but that resulted on a high decrease on benchmark. Now Express is able to deal with paypal ipn and windows web services. Thanks again! when did you plan to make this available on npm?

tj commented 13 years ago

0.3.1 should be available now

zdwalter commented 12 years ago

i don't understand why need to replace the '+' with ' '. Any one can tell? Thanks.