Properties with names colliding with immutable methods (e.g. update, merge) are getting lost

[tad-lispy]

Consider this object:

{ links:
   { update: 'http://api.dev/...',
     delete: 'http://api.dev/...' } }

If one pass it to seamless-immutable, this will be returned:

{ links:
   { delete: 'http://api.dev/...' } }

[tad-lispy]

Maybe it's a good reason to revisit #81?

[tad-lispy]

Given prior discussion at #131 it seems like correct solution to this problem is to add Immutable specific methods to the constructor (aka making them static). This was previously proposed in #81 .

---

Do you think that methods should be defined only on the constructor, or also on instances?

The constructor only approach seems more elegant, but would be a serious breaking change.

With constructor + instance approach, we still have to figure out how collisions with original object properties should be resolved. Also things like that:

immutable_instance.merge({ without: "data" })

[mariusk]

I'm late to the party, but I just migrated a code base from immutable.js to this and was immediately hit by one member deep in a object hierarchy named set. On the one hand, migration was easy as some of the common API members have identical names and signatures as immutable.js (setIn), but unless anybody else have better ideas I would suggest some unlikely prefix or postfix for all method names or similar (e.g. setInImm, imSetIn or something like that).

[suchipi]

Ran into this same issue today (with update). It'd be nice if we could do:

import Immutable, { merge } from "seamless-immutable";

let options = Immutable({ merge: "true" });
options = merge(options, { something: "else" });
console.log(options); // { merge: true, something: "else" }

[ghost1542]

I have just submitted #158 to add static methods support in the Immutable constructor.

var obj = Immutable.from({});
Immutable.setIn(obj, ['key'], value);

[rtfeldman]

Fixed by https://github.com/rtfeldman/seamless-immutable/pull/158

[suchipi]

Does #158 actually fix it? Just adding static methods won't prevent name collision from the instance methods that are also still present. The collision won't be truly fixed until the instance methods are also removed, which would be a breaking change (but a welcome one, in my opinion).

[rtfeldman]

Gotcha...I think a feature request for "avoid clobbering existing fields" makes sense.

Want to open one? 😄

[ghost1542]

Right so at least now there is a way to avoid this bug using the new syntax. Disabling the old syntax is pretty simple but indeed not a small breaking change.

Perhaps as a first step the documentation can be updated to only document the new static syntax? We can also easily insert a "Will be deprecated" warning in the browser javascript console for the devel build when users continue using the existing instance syntax for a while?

I am not sure if it is worth to actually disable that syntax in the near future as the disruption of this change is important for current users compared to the likeliness that a user experiences this bug.

[ghost1542]

Oh I see. I got this wrong, it is the custom properties that are getting lost, not the seamless-immutable instance methods. To allow overriding those, I guess we could set writable to true here. This would make new static methods fully working against all cases.

    Object.defineProperty(target, methodName, {
      enumerable: false,
      configurable: false,
      writable: true,
      value: value
    });

I am happy to submit this, documentation update, test units, and deprecation warning, if @rtfeldman thinks this is a good approach to this problem.

[ghost1542]

PR to complete this fix https://github.com/rtfeldman/seamless-immutable/pull/160