Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplifies shouldComponentUpdate (open discussion) #78

Merged
merged 8 commits into from
Mar 5, 2015
Merged

Simplifies shouldComponentUpdate (open discussion) #78

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fec0172
Simplifies shouldComponentUpdate and switch from isImmutable to isCom…
mikaelbr Feb 28, 2015
b38f53a
Refactors and dries up code
mikaelbr Feb 28, 2015
7eee6dd
Reducing work by moving code to prioritized order
mikaelbr Feb 28, 2015
61c9641
Fixes equality check for immutables
mikaelbr Feb 28, 2015
e29bdef
Renames parameter for compare
mikaelbr Feb 28, 2015
fe78199
Also checks on state for quick exit on SCU
mikaelbr Feb 28, 2015
77cf4e3
tests for SCU quick exit
dashed Mar 1, 2015
6a83813
Merge pull request #79 from Dashed/shouldComponentUpdateSimplified
mikaelbr Mar 1, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 35 additions & 65 deletions shouldupdate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,36 +67,22 @@ function factory (methods) {
return shouldComponentUpdate;

function shouldComponentUpdate (nextProps, nextState) {
var isNotIgnorable = not(or(isStatics, isChildren));

var nextCursors = filter(nextProps, isNotIgnorable),
currentCursors = filter(this.props, isNotIgnorable);

var nextCursorsKeys = Object.keys(nextCursors),
currentCursorsKeys = Object.keys(currentCursors);

if (currentCursorsKeys.length !== nextCursorsKeys.length) {
if (debug) debug.call(this, 'shouldComponentUpdate => true (number of cursors differ)');
return true;
if (nextProps === this.props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if assert(nextProps === this.props && nextState !== this.state)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right.

if (debug) debug.call(this, 'shouldComponentUpdate => false (equal input)');
return false;
}

if (hasDifferentKeys(currentCursorsKeys, currentCursors, nextCursors)) {
if (debug) debug.call(this, 'shouldComponentUpdate => true (cursors have different keys)');
return true;
}

if (hasChangedCursors(currentCursors, nextCursors)) {
if (debug) debug.call(this, 'shouldComponentUpdate => true (cursors have changed)');
return true;
}
var isNotIgnorable = not(or(isStatics, isChildren));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this outside of shouldComponentUpdate for caching. No need to reconstruct this higher-order function on every render.

var nextProps = filter(nextProps, isNotIgnorable),
currentProps = filter(this.props, isNotIgnorable);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this below _isEqualState block for better placement:

    var nextProps    = filter(nextProps, isNotIgnorable),
        currentProps = filter(this.props, isNotIgnorable);

Reasoning is that it could be a pointless computation if !_isEqualState(this.state, nextState) === true.


if (!_isEqualState(this.state, nextState)) {
if (debug) debug.call(this, 'shouldComponentUpdate => true (state has changed)');
return true;
}

if (hasChangedProperties(currentCursors, nextCursors)) {
if (debug) debug.call(this, 'shouldComponentUpdate => true (properties have changed)');
if (!_isEqualProps(currentProps, nextProps)) {
if (debug) debug.call(this, 'shouldComponentUpdate => true (props have changed)');
return true;
}

Expand All @@ -105,33 +91,6 @@ function factory (methods) {
return false;
}

function hasChangedCursors (current, next) {
current = filter(current, _isCursor);
next = filter(next, _isCursor);

var nextKeys = Object.keys(current),
currentKeys = Object.keys(next);

var nextLength = nextKeys.length;

if (nextLength !== currentKeys.length) {
return true;
}

for (var i = 0; i < nextLength; i++) {
if (!_isEqualCursor(current[nextKeys[i]], next[currentKeys[i]])) {
return true;
}
}
return false;
}

function hasChangedProperties (current, next) {
current = filter(current, not(_isCursor));
next = filter(next, not(_isCursor));
return !_isEqualProps(current, next);
}

/**
* Predicate to check if state is equal. Checks in the tree for immutable structures
* and if it is, check by reference. Does not support cursors.
Expand All @@ -147,10 +106,17 @@ function factory (methods) {
*/
function isEqualState (value, other) {
return isEqual(value, other, function (current, next) {
if (_isImmutable(current) && _isImmutable(next)) {
if (current == next) {
return true;
}

var isCurrentImmutable = _isImmutable(current);
var isNextImmutable = _isImmutable(next);

if (isCurrentImmutable && isNextImmutable) {
return current === next;
}
if (_isImmutable(current) || _isImmutable(next)) {
if (isCurrentImmutable || isCurrentImmutable) {
return false;
}
return void 0;
Expand All @@ -172,18 +138,30 @@ function factory (methods) {
*/
function isEqualProps (value, other) {
return isEqual(value, other, function (current, next) {
if (_isCursor(current) && _isCursor(next)) {
if (current == next) {
return true;
}

var isCurrentCursor = _isCursor(current);
var isNextCursor = _isCursor(next);

if (isCurrentCursor && isNextCursor) {
return _isEqualCursor(current, next);
}
if (_isCursor(current) || _isCursor(next)) {
if (isCurrentCursor || isNextCursor) {
return false;
}
if (_isImmutable(current) && _isImmutable(next)) {

var isCurrentImmutable = _isImmutable(current);
var isNextImmutable = _isImmutable(next);

if (isCurrentImmutable && isNextImmutable) {
return current === next;
}
if (_isImmutable(current) || _isImmutable(next)) {
if (isCurrentImmutable || isCurrentImmutable) {
return false;
}

return void 0;
});
}
Expand Down Expand Up @@ -236,7 +214,6 @@ function factory (methods) {
}
}


/**
* Predicate to check if a potential is an immutable structure or not.
* Override through `shouldComponentUpdate.withDefaults` to support different cursor
Expand All @@ -245,7 +222,7 @@ function factory (methods) {
* @param {maybeImmutable} value to check if it is immutable.
*
* @module shouldComponentUpdate.isImmutable
* @returns {Object|Number|String|Boolean}
* @returns {Boolean}
* @api public
*/
var IS_ITERABLE_SENTINEL = '@@__IMMUTABLE_ITERABLE__@@';
Expand All @@ -269,7 +246,6 @@ function unCursor(cursor) {
return cursor.deref();
}


/**
* Predicate to check if `potential` is Immutable cursor or not (defaults to duck testing
* Immutable.js cursors). Can override through `.withDefaults()`.
Expand All @@ -281,13 +257,7 @@ function unCursor(cursor) {
* @api public
*/
function isCursor (potential) {
return potential && typeof potential.deref === 'function';
}

function hasDifferentKeys (currentCursorsKeys, currentCursors, nextCursors) {
return !currentCursorsKeys.every(function existsInBoth (key) {
return typeof currentCursors[key] !== 'undefined' && typeof nextCursors[key] !== 'undefined';
});
return !!(potential && typeof potential.deref === 'function');
}

function not (fn) {
Expand Down
8 changes: 4 additions & 4 deletions tests/debug-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ describe('debug', function () {

var localComp = shouldComponentUpdate.withDefaults();
localComp.debug(function logger (message) {
message.should.contain('true (number of cursors differ)');
message.should.contain('true (props have changed)');
done();
});

Expand All @@ -207,7 +207,7 @@ describe('debug', function () {

var localComp = shouldComponentUpdate.withDefaults();
localComp.debug(function logger (message) {
message.should.contain('true (cursors have different keys)');
message.should.contain('true (props have changed)');
done();
});

Expand All @@ -228,7 +228,7 @@ describe('debug', function () {

var localComp = shouldComponentUpdate.withDefaults();
localComp.debug(function logger (message) {
message.should.contain('true (cursors have changed)');
message.should.contain('true (props have changed)');
done();
});

Expand All @@ -254,7 +254,7 @@ describe('debug', function () {
it('should log on properties have changed', function (done) {
var localComp = shouldComponentUpdate.withDefaults();
localComp.debug(function logger (message) {
message.should.contain('true (properties have changed)');
message.should.contain('true (props have changed)');
done();
});

Expand Down
14 changes: 7 additions & 7 deletions tests/shouldComponentUpdate-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,14 @@ describe('shouldComponentUpdate', function () {
it('when state have immutable structures', function () {
shouldUpdate({
state: { foo: Immutable.List.of(1) },
nextState: { foo: Immutable.List.of(1) },
nextState: { foo: Immutable.List.of(2) },
});
});

it('when props have immutable structures', function () {
shouldUpdate({
cursor: { foo: Immutable.List.of(1) },
nextCursor: { foo: Immutable.List.of(1) },
nextCursor: { foo: Immutable.List.of(2) },
});
});

Expand Down Expand Up @@ -280,10 +280,10 @@ describe('shouldComponentUpdate', function () {
it('should have overridable isEqualCursor', function (done) {
var data = Immutable.fromJS({ foo: 'bar', bar: [1, 2, 3] });
var local = component.withDefaults({
isEqualCursor: function () { done() }
isEqualCursor: function () { done(); return true; }
}).shouldComponentUpdate;

shouldUpdate({
shouldNotUpdate({
cursor: Cursor.from(data, ['foo']),
nextCursor: Cursor.from(data, ['bar'])
}, local);
Expand Down Expand Up @@ -333,13 +333,13 @@ describe('shouldComponentUpdate', function () {
localComponent.debug.should.be.a('function');
});

it('should have overridable isEqualCursor', function (done) {
it('should have overridable isEqualCursor', function () {
var data = Immutable.fromJS({ foo: 'bar', bar: [1, 2, 3] });
var local = shouldComponentUpdate.withDefaults({
isEqualCursor: function () { done() }
isEqualCursor: function () { return true; }
});

shouldUpdate({
shouldNotUpdate({
cursor: Cursor.from(data, ['foo']),
nextCursor: Cursor.from(data, ['bar'])
}, local);
Expand Down