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

Use nullary objects #58

Closed eivindfjeldstad closed 11 years ago

eivindfjeldstad commented 11 years ago

Don't allow Object prototype to be modified. Test will fail in IE < 9

wavded commented 11 years ago

nullary objects also remove toString() and valueOf() which means you can't compare values with ==

var object = Object.create(null)
object == 'thingy'
TypeError: Cannot convert object to primitive value

=== will work but i imagine having no Object.prototype anymore is causing issues for others (like in the reference above)

/cc @visionmedia

tj commented 11 years ago

yarg :( silly javascript

eivindfjeldstad commented 11 years ago

An alternative could be to keep a blacklist of properties. Or even attatch the proto after the fact. Like

var obj = Object.create(null);
// parse querystring
obj.__proto__ = {}

Might be bad for performance though

wavded commented 11 years ago

if I may inquire, what was the rationale for using nullary objects?

Object.create(null) is significantly slower than {}, not sure how hot this code path is in this case though:

http://jsperf.com/object-create-proto-after

tj commented 11 years ago

__proto__ is plenty fast enough if we want to go that route, plus @wavded it's definitely much slower but at several million ops/s that doesn't matter

wavded commented 11 years ago

yeah fair enough, i'm cool with __proto__

eivindfjeldstad commented 11 years ago

I have a feeling this is going to come up again and again. As long as Object.prototype is safe, I'm happy.

wavded commented 11 years ago

Yeah I agree

tj commented 11 years ago

0.6.3 has the fix