React.addons.update should return the input object when we replace a value by itself

[slorber]

This code shows the problem

var root = {
    prop: "propValue",
    nested: {
        nestedProp: "nestedPropValue"
    }
}

var rootCopy = React.addons.update(root,{
    nested: { $set: root.nested }
});

var rootCopy2 = React.addons.update(root,{
    nested: { nestedProp : {$set: root.nested} }
});

console.debug("root === rootCopy",root === rootCopy);
console.debug("root.prop === rootCopy.prop",root.prop === rootCopy.prop);
console.debug("root.nested === rootCopy.nested",root.nested === rootCopy.nested);

console.debug("root === rootCopy2",root === rootCopy);
console.debug("root.prop === rootCopy2.prop",root.prop === rootCopy2.prop);
console.debug("root.nested === rootCopy2.nested",root.nested === rootCopy2.nested);

root === rootCopy false app.js:46
root.prop === rootCopy.prop true app.js:47
root.nested === rootCopy.nested true app.js:48

root === rootCopy2 false app.js:50
root.prop === rootCopy2.prop true app.js:51
root.nested === rootCopy2.nested false 

I woould expect all these equality to be true.

I think this is the point of immutability/structural sharing to not create useless objects when their inner properties are equally the same.

var object = {}
object = React.addons.update(object, { prop: { $set: propValue} });
object = React.addons.update(object, { prop: { $set: propValue} });

On the 2nd call, it makes sense that the object returned is the object from the arguments.

Ensuring this can help to avoid unexpected behavior in shouldComponentUpdate and prevent some useless renderings that I have encountered (when user clicks twice on the same menu element for exemple)

[chenglou]

(If I'm not mistaken, rootCopy2 should have been nested: { nestedProp : {$set: root.nested.nestedProp} }. That might have been a typo?)

I'm fairly positive this is intentional. update is very lightweight and not meant to be used as a primary data structure for your apps. More for fine tuning a few shouldComponentUpdates (@petehunt). Thus it doesn't try to do smart structural sharing. You weren't supposed to be able to use === on it, more like manually comparing fields.

mori's what you're looking for perhaps.

[syranide]

@chenglou I'm not sure if I agree, this is not about structural sharing, but simply about detecting equivalence of old/new properties before mutating. It shouldn't be many lines of code. Mori is a library for structural sharing, which is a vastly more complex problem and I sincerely doubt any reasonably common JS app use-case could at all turn that overhead into a benefit.

[slorber]

Yes @syranide OI don't want to use a full structural sharing library like Mori unless I have to deal with very large lists or things like that...

And yes, it's really easy to implement.
In my own code I fixed this behavior with:

function updatePathValue(object,path,value) {
        if ( getPathValue(object,path) === value ) {
            return object;
        }
        var objectPath = pathToReactUpdatePath(path,{$set: value});
        return React.addons.update(object, objectPath);
}

[chenglou]

I guess I see the point then? Although in your example you did end up allocating a new object anyways so the benefit of not allocating is lost. If you feel === is still important then feel free to submit a PR (possibly without the $set allocation).

BTW, update is getting deprecate (as in, pulled into its own repo that other people can maintain) in favour of immutable-js. We could get this in before that happens. @petehunt

[slorber]

Just wanted to mention that.
Anyway i've added myself the === check in my framework that uses React updates for now
https://github.com/stample/atom-react/blob/master/src/atom/atomUtils.js#L72

I can wait for the new repo to fix this, or maybe I'll try to migrate our apps to immutable-js

[davidmason]

@chenglou @syranide I am happy to make a PR this weekend on this if it is wanted. It should be possible to detect cases in $set, $merge and $apply that can return the same object, although the latter two may be more of a challenge. I can make separate PRs for each command if either of the latter two start to look too slow or messy.

[gaearon]

We are handing over update() to the community so I’m closing this.
Please see my note in #6353 (comment).