blueprint
blueprint copied to clipboard
Fix shouldComponentUpdate in OverflowList
Checklist
- [ ] Includes tests
- [ ] Update documentation
Changes proposed in this pull request:
Potentially fixes two issues: incorrect logic & lack of prop equality checking.
Potentially incorrect logic:
Original expression: !(this.state !== nextState && shallowCompareKeys(this.state, nextState))
With DeMorgan's laws, this can be written as: (this.state === nextState || !shallowCompareKeys(this.state, nextState))
This is saying that the component SHOULD update iff: the states are equal by reference OR the keys are NOT shallow equal.
I don't believe this was the intended logic, based on the above comments -- I imagine we don't actually want to re-render if the states have referential equality.
I think the intended logic was: this.state !== nextState && !shallowCompareKeys(this.state, nextState)
or equivalently !(this.state === nextState || shallowCompareKeys(this.state, nextState))
Lack of prop equality check
If the render's callbacks change reference equality we aren't guaranteed a re-render like we should. This lead to strange bugs, and where breakpoints were accidentally fixing the race conditions happening here.
Reviewers should focus on:
- Whether the new logic I wrote is the intended behavior.
- If there was some deeper reason for omitting the reference equality check on props?
Screenshot
Thanks for your interest in palantir/blueprint, @braeden! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.