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 194 forks source link

Different results depending on how you call merge #148

Closed chadly closed 5 years ago

chadly commented 8 years ago
var Immutable = require("seamless-immutable");

var user = Immutable({ username: "homer.simpson"});

var mergedIncorrectly = user.merge([{
    username: "HoMeR.SiMpSoN",
    city: "Springfield"
}, {
    username: "homer.simpson",
    kids: 3
}]);

var mergedCorrectly = user.merge({
    username: "HoMeR.SiMpSoN",
    city: "Springfield"
}).merge({
    username: "homer.simpson",
    kids: 3
});

console.log({
    mergedIncorrectly: mergedIncorrectly,
    mergedCorrectly: mergedCorrectly
});

Output:

{ mergedIncorrectly: { username: 'HoMeR.SiMpSoN', city: 'Springfield', kids: 3 },
  mergedCorrectly: { username: 'homer.simpson', city: 'Springfield', kids: 3 } }

This is perplexing to me as it doesn't seem to merge the username property correctly when calling the array version of merge (but it does get the kids property) while merging the same objects with two separate calls to merge works fine.

According to the docs, these two should be equivalent:

Multiple objects can be provided in an Array in which case more merge invocations will be performed using each provided object in turn.

hilkeheremans commented 7 years ago

I haven't checked or tried it out, but my first guess would be that in the "incorrect" case, seamless-immutable, when provided with an array in merge, merges from the last element to the first. This would cause homer.simpson to be overwritten with its l33t counterpart. The opposite is happening in the "correct" case since you are making two distinct merge calls which get called in that order.

Might be a good idea to explicitly call that out in the docs.

crudh commented 7 years ago

@chadly that's strange. I added the following test case:

    it("merges arrays with the same result as chained merges", function() {
      var expected = Immutable({ username: "homer.simpson", city: "Springfield", kids: 3 })

      var original = Immutable({ username: "homer.simpson"});
      var toMerge1 = { username: "HoMeR.SiMpSoN", city: "Springfield" };
      var toMerge2 = { username: "homer.simpson", kids: 3 };

      var actualChainedMerges = original.merge(toMerge1).merge(toMerge2);
      var actualArrayMerge = original.merge([toMerge1, toMerge2]);

      TestUtils.assertJsonEqual(actualChainedMerges, expected);
      TestUtils.assertJsonEqual(actualArrayMerge, expected);
    });

and it works and produces the correct result in both cases.

crudh commented 5 years ago

Closing due to inactivity.