AmpersandJS / ampersand-state

Core state management object.
MIT License
141 stars 75 forks source link

Possible need for semver major #226

Closed dhritzkiv closed 8 years ago

dhritzkiv commented 8 years ago

With the optimisation changes made in c6a16ed689dea762b2b7c67368ae8c4456d506e7, I've come across an edge case that may require a major release (and possible deprecation of v4.9.0+).

The optimisations included a transition away from using for (attr in obj) to Object.keys(obj) in order to iterate the object properties. While this is more efficient, and what is necessary nearly all of the time, it changes which properties are iterated over. Essentially, for…in loops over all enumerable properties, including those inherited through the prototype chain, while Object.keys only returns own attributes that are enumerable.

See this small example on tonicdev

This switch also affects instances of certain constructors like Blob/File, among others. for...in loops through those instances' keys (size, name, type, slice), while Object.keys does not (at least in Safari). I actually don't know why, but I suspect this has to do with getters/setters and the Browser specific-implementation nature of these instances. Here's a another small example working with Blobs, on jsbin Since the JSBin example isn't working consistently, I've inlined the code and results:


var blob = new Blob(["hello"]);

console.log(blob.size);//5

console.log("Object.keys", Object.keys(blob)); // "Object.keys" []

var attr;
var attrs = [];

for (attr in blob) {
    attrs.push(attr);
}

console.log("for in", attrs); // "for in" ["size", "slice", "type"]

This change actually bit me while working in my ampersand-file-drop-view module which works with Blob/File objects, and setting those blobs onto a State model. (I'm working to make changes in that module to be more explicit about setting the appropriate properties).

Again, these are edge cases, but functionality breaking nonetheless, warranting a major semver release, as annoying as it is. It would also warrant a note in a README.

wraithgar commented 8 years ago

Yes unfortunately Object.keys does not return anything up the prototype chain. This does constitute a change in behavior (even if using undocumented/edge case behavior). Versions are cheap.

If you'd be willing to add a note to state.set to the effect of 'prototypical properties are not evaluated here' only in a more friendly language, that'd be the major version PR imho.