rtfeldman / seamless-immutable

Immutable data structures for JavaScript which are backwards-compatible with normal JS Arrays and Objects.
BSD 3-Clause "New" or "Revised" License
5.37k stars 195 forks source link

Use existing prototypes #199

Closed nadavsinai closed 7 years ago

nadavsinai commented 7 years ago

This PR comes from trying to use this lib in a very OO environment where trees and trees of inheritable member functions exist on the prototypes. I've altered the default prototype to inherit from the runtime proto notice that in es6 this has finally been admitted into the language and is no longer considered a bad practice to my best knowledge

rtfeldman commented 7 years ago

notice that in es6 this has finally been admitted into the language and is no longer considered a bad practice to my best knowledge

Unfortuantely not.

More to the point, I appreciate the effort here, but I'm not okay with this from a design perspective. I think consistency of immutability is the right default, and making it so that prototype comes along by default means that it's super easy to call Immutable on something and be surprised that it's not actually immutable (because the prototype methods access mutable state).

I'm okay allowing this on an opt-in basis, but I am not okay with making it the default.

Sorry you spent so much time on this! 😅

nadavsinai commented 7 years ago

First of all thank you for your prompt response, I think that this PR should be split into a few, there's a couple of issues: 1) the first commit Algotec/seamless-immutable@dd5c000 fixes a bug where the config object is ignored when calling set/setIn, this is unrelated to taking existing prototypes and continues your philosophy of making the user explicitly declare the prototype - though making it possible for props to have prototypes at all. 2) the 2nd and third commits here are the thing we differ about, I come from a perspective where the developer that chooses to use this library has awareness of immutability, the responsibility to make the prototype methods not alter the same object but to return a new one should be documented, but I don't think that loosing object prototypes should be a side-effect of calling for immutability, especially since you make it so unpredictable - main objects can have prototypes (with config) but object props can not? Calling Set will remove prototypes? now about the possibility of using existing prototypes, this can be an opt-in configuration, just like the ability to stop the run-time prototype modification you are doing already. 3) the call to proto can be changed to

 (typeof Object.getPrototypeOf =='function') ? Object.getPrototypeOf(obj) : obj.__proto__

which brings you into compatibility but shows that you've tried to make it use the official syntax... there is no harm in getting protoypes - read the harm is done when changing prototypes write - and this was something that can be (and should) not be done by your lib, imagine something like getting existing prototype, then using object assign to create a new object with all prototype methods and seamless-immutable methods, then using this new prototype for the object using Object.create

let oldProto =  (typeof Object.getPrototypeOf =='function') ? Object.getPrototypeOf(obj) : obj.__proto__;
let newProto = Object.assign({},oldProto ,immtableObjectProto);
let clone = Object.create(newProto,obj);

I can fix this PR to do this, and make this a configurable option, defaults can be as you wish.