Misleading function shallowEqual()

[tonix-tuft]

Hi and thank you for this HOC. I also read your article https://medium.com/myheritage-engineering/how-to-greatly-improve-your-react-app-performance-e70f7cbbb5f6 which has been pretty helpful.

I only have one question regarding the shallowEqual function:

export function shallowEqual(thisProps, nextProps, thisState, nextState) {
    return !shallowEqualState(thisState, nextState) || !shallowEqualWithoutReactElements(thisProps, nextProps);
}

Right now, it returns true when the component should update (something has changed), and false otherwise. On the other hand, shallowEqualState and shallowEqualWithoutReactElements both work in the opposite way: they return false when there's a change (something has changed), and true otherwise.

That said, shouldn't shallowEqual use logical AND instead of OR in its body? E.g.:

export function shallowEqual(thisProps, nextProps, thisState, nextState) {
    return shallowEqualState(thisState, nextState) && shallowEqualWithoutReactElements(thisProps, nextProps);
}

This way, if shallowEqual returns true, it means that the props/state are equal and that the component should not update. Whereas, if it returns false, it would mean that something has changed and the component should update.

And then, someone using shallowEqual would have to negate it in order to know that something has changed and therefore a component should update:

            ...
            if (!super.shouldComponentUpdate || super.shouldComponentUpdate(nextProps, nextState)) {
                shouldUpdate = !shallowEqual(this.props, nextProps, this.state, nextState); // If shallowEqual returns `false`, it means that the prev props or state are not equal to the next props or state and we should therefore update...
            }
            ...

What do you think?

[NoamELB]

I guess you are right and it should be named shouldUpdate instead of shallowEqual