search

montagejs / collections

This package contains JavaScript implementations of common data structures with idiomatic interfaces.
http://www.collectionsjs.com
Other
2.09k stars 185 forks source link

Must you modify core prototypes? e.g. Object.prototype #70

Closed buchanae closed 10 years ago

buchanae commented 10 years ago

I notice you "shim" a bunch of things onto core prototypes, e.g. Object.addEach. Is that really necessary? Isn't it taboo to modify Object.prototype?

Can you at least point me to the ECMAScript standard that is planning to implement Object.addEach and all its friends? I can't find it.

I want to use your library, but honestly I get a bad feeling about all this modification of core prototypes.

kriskowal commented 10 years ago

There is not standard that introduces Object.addEach, Object.compare, Object.equals, or Object.clone. Object.is was proposed, but has not been reified. It is safe to assume that Object.hash will never become a standard, and is only necessary for polyfilling Map and Set. When it comes to polyfills and prollyfills, it is always hard to tell when to lead and when to follow. Array.prototype.find is one polyfill I got wrong, (I correctly anticipated its standardization but did not anticipate the same semantics) which has been fixed for the unreleased v2 branch.

TC39 looks to code in the wild for patterns to standardize. As such, there is a bit of a chicken and egg problem. See https://www.youtube.com/watch?v=xL3xCO7CLNM

To adopt Collections does mean buying into certain ideas for future standards.

kriskowal commented 10 years ago

(To be clear, I closed the issue because I don’t foresee acting on this. It is not to say that the issue is closed to discussion.)

buchanae commented 10 years ago

Thanks for the quick and clear response. At least this development decision/philosophy is documented here now, and here for discussion.

Personally, I disagree. You're imposing unnecessary risk on codebases by making global modifications. Will it result in many terrible headaches, or a few minor ones? I can't say, but I'm fairly confident it will bite someone. If it can be done a different way without much trouble and reduce the risk, why not do it that way?

JHumphreyJr commented 10 years ago

This package definitely interferes with the Array.prototype.find polyfill. Any ETA on a V2 release to resolve this issue?

kriskowal commented 10 years ago

I can’t offer an estimate, but I am moving in this direction on v2 https://github.com/montagejs/collections/commit/7c674d49c04955f01bbd2839f90936e15aceea2f

timruffles commented 8 years ago

I think it'd be at least good to document this. It's definitely a "principle of least surprise" violation (e.g I was very confused by seeing [].set() in the source) considering the JS ecosystem aversion to extending core types.