danmactough / node-feedparser

Robust RSS, Atom, and RDF feed parsing in Node.js
Other
1.97k stars 192 forks source link

VERY IMPORTANT! Do not override Array.prototype! #9

Closed bminer closed 12 years ago

bminer commented 12 years ago

See https://github.com/danmactough/node-feedparser/blob/master/lib/utils.js#L70

Please do not change the Array prototype. It is very bad and causes major headaches that are very difficult to debug! No exception here.

Look at what this does:

var x = [1, 2, 3];
for(var i in x) console.log(i); //Prints 0, 1, 2
require('feedparser');
for(var i in x) console.log(i); //Prints 0, 1, 2, and unique!! NOOOOO!!!!

Please fix this at your earliest convenience. Thanks!

bminer commented 12 years ago

Please also... do not be tempted to use Object.defineProperty even though it can be used to make unique non-enumerable.

danmactough commented 12 years ago

Probably a good idea not to use for..var..in loops on an Array -- there is forEach for that. But there is no need for this library to be so inconsiderate, either. I'll change this as soon as possible. ------Original Message------ From: Blake Miner To: Daniel MacTough Subject: [node-feedparser] VERY IMPORTANT! Do not override Array.prototype! (#9) Sent: Feb 20, 2012 12:38 PM

See https://github.com/danmactough/node-feedparser/blob/master/lib/utils.js#L70

Please do not change the Array prototype. It is very bad and causes major headaches that are very difficult to debug! No exception here.

Look at what this does:

var x = [1, 2, 3];
for(var i in x) console.log(i); //Prints 0, 1, 2
require('feedparser');
for(var i in x) console.log(i); //Prints 0, 1, 2, and unique!! NOOOOO!!!!

Please fix this at your earliest convenience. Thanks!


Reply to this email directly or view it on GitHub: https://github.com/danmactough/node-feedparser/issues/9

bminer commented 12 years ago

Thank you! Sorry if I seemed a bit edgy on the subject; maybe "very important" in all caps was a bit excessive. :P I had to do grep -r "Array.prototype.unique" /usr/lib/node_modules/ to find the culprit. :)

danmactough commented 12 years ago

No worries. Happy to see that someone else is using the library. Fixed.

bminer commented 12 years ago

Thank you, thank you!

bminer commented 12 years ago

Nice lib, btw! What a time saver!