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

Implement optional deep compare for .set, .setIn and .replace #145

Closed ghost1542 closed 7 years ago

ghost1542 commented 8 years ago

This feature is helpful to leverage the usefulness of immutable objects when they need to be refreshed from external data. Typical example is refreshing immutable data from an API call, and have pure React components watching for granular changes in those objects.

obj.merge(data, {deep: true}) almost provides the desired behavior, except that deprecated object keys from previous immutable objects can pollute new objects, and lead to unexpected behavior.

This pull request addresses this use case by allowing the use of {deep: true} on .set and .setIn and a new .replace() method. When using those, merge() is used to merge mergeable objects, with an extra {mode: 'replace'} parameter to delete deprecated keys on the new object.

The feature was not implemented for .update and .updateIn in the expectation that custom functions can handle deep merge and granular changes themselves in a faster fashion.

This pull request also includes test units (ran extensively) and fixes some edge cases with NaN, as well as updating lodash to fix bugs where it was at fault.

Performances for deep .merge() should be unchanged, deep replace is obviously expected to be a bit slower than normal merge, tested with code below.

Thank you for this fantastic immutable library :-)

// Benchmark obj.merge()

function testMerge(obj, keys) {
    var sum = 0;
    var now = new Date().getTime();
    for (var i = 0, n = keys.length; i < n; i++) {
        var key = keys[i];
        obj[key] = obj[key].merge({b: {a: 'test'}}, {deep: true});
        sum += obj[key]['a'];
    }
    console.log(sum || 0);
    return new Date().getTime() - now;
}

var nbLoops = 10000;
var runs = 5;
var time = 0;
for (var i = 0; i < runs; i++) {
    var ii = 0;
    var keys = [];
    var obj = {};
    while (ii < nbLoops) {
        obj[ii] = Immutable.from({
            'a': ii,
            'b': {
                a: ii
            }
        });
        keys.push(ii);
        ii++;
    }
    time += testMerge(obj, keys);
}
time = Math.round(time / runs)
console.log('Immutable merge deep took ' + time + ' milliseconds for ' + nbLoops + ' loops on average')
ghost1542 commented 8 years ago

Tests appear to be passing on travis, seems like it is getting some configuration error after running them.

Zuul tried to run tests in saucelabs, however no saucelabs credentials were provided.
ghost1542 commented 7 years ago

@rtfeldman Bump, just checking if there is any chance this can be included in the library :-) I'm around if you have any comments.

rtfeldman commented 7 years ago

Seems reasonable, and deep-equals seems like a trivially small dependency to add.

If you wouldn't mind resolving the merge conflicts, I'd be happy to merge this!

ghost1542 commented 7 years ago

@rtfeldman Just rebased with support for static methods in tests and .replace. Should be good to go.

Seems reasonable, and deep-equals seems like a trivially small dependency to add.

Yup and actually it's only a devDependency for test units. It is not used in the actual seamless-immutable code (tried to keep it as optimized as possible).