immutable-js / immutable-js

Immutable persistent data collections for Javascript which increase efficiency and simplicity.
https://immutable-js.com
MIT License
32.91k stars 1.79k forks source link

fromJSGreedy / fromJSOrdered : ignore immutable collections #1703

Open Enteee opened 5 years ago

Enteee commented 5 years ago

I would suggest changing the fromJSGreedy / fromJSOrdered functions, documented in the wiki, to exclude immutable collections provided by this module. This is in order to preserve value identity on subsequent calls.

const immutableJS = require('immutable');
function fromJSGreedy (js) {
  return typeof js !== 'object' || js === null 
    ? js : Array.isArray(js)
      ? immutableJS.Seq(js).map(fromJSGreedy).toList()
      : immutableJS.Seq(js).map(fromJSGreedy).toMap();
}

obj1 = fromJSGreedy({a:1})
obj2 = fromJSGreedy(obj1)
obj1 == obj2 // false

Suggestion:

const immutableJS = require('immutable');
function fromJSGreedy (js) {
  return typeof js !== 'object' || js === null || js instanceof immutableJS.Collection
    ? js : Array.isArray(js)
      ? immutableJS.Seq(js).map(fromJSGreedy).toList()
      : immutableJS.Seq(js).map(fromJSGreedy).toMap();
}

obj1 = fromJSGreedy({a:1})
obj2 = fromJSGreedy(obj1)
obj1 == obj2 // true
MichaelTamlyn commented 5 years ago

We did something similar. Our implementation:

export function fromJSGreedy(js) {
    switch (true) {
        case ((typeof js) !== "object"): // primitive or function
        case (js === null):
        case (js instanceof Date): // Date objects should not be converted
        case (Record.isRecord(js)): // Immutable Record
            return js;

        case (isCollection(js)):
            return js.map(fromJSGreedy); // Immutable Collection (Map/List/Set/Seq/etc)

        case (Array.isArray(js)): // turn arrays into Lists
            return Seq(js).map(fromJSGreedy).toList();

        default: // turn POJOs and class instances into Maps
            return Seq(js).map(fromJSGreedy).toMap();
    }
}