Skip to content

Commit

Permalink
Merge pull request #78 from omniscientjs/shouldComponentUpdateSimplified
Browse files Browse the repository at this point in the history 
Simplifies shouldComponentUpdate (open discussion)
  • Loading branch information
@mikaelbr
mikaelbr committed Mar 5, 2015
2 parents 5d293ef + 6a83813 commit 9f3715c
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 90 deletions.
115 changes: 36 additions & 79 deletions shouldupdate.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
var filter = require('lodash.pick'),
isEqual = require('lodash.isequal');

var isNotIgnorable = not(or(isStatics, isChildren));

/**
* Directly fetch `shouldComponentUpdate` mixin to use outside of Omniscient.
* You can do this if you don't want to use Omniscients syntactic sugar.
Expand Down Expand Up @@ -67,36 +69,21 @@ 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 (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;
if (nextProps === this.props && nextState === this.state) {
if (debug) debug.call(this, 'shouldComponentUpdate => false (equal input)');
return false;
}

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)');
var nextProps = filter(nextProps, isNotIgnorable),
currentProps = filter(this.props, isNotIgnorable);

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

Expand All @@ -105,33 +92,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,13 +107,8 @@ function factory (methods) {
*/
function isEqualState (value, other) {
return isEqual(value, other, function (current, next) {
if (_isImmutable(current) && _isImmutable(next)) {
return current === next;
}
if (_isImmutable(current) || _isImmutable(next)) {
return false;
}
return void 0;
if (current === next) return true;
return compare(current, next, _isImmutable, isEqualImmutable);
});
}

Expand All @@ -172,19 +127,12 @@ function factory (methods) {
*/
function isEqualProps (value, other) {
return isEqual(value, other, function (current, next) {
if (_isCursor(current) && _isCursor(next)) {
return _isEqualCursor(current, next);
}
if (_isCursor(current) || _isCursor(next)) {
return false;
}
if (_isImmutable(current) && _isImmutable(next)) {
return current === next;
}
if (_isImmutable(current) || _isImmutable(next)) {
return false;
}
return void 0;
if (current === next) return true;

var cursorsEqual = compare(current, next, _isCursor, _isEqualCursor);
if (cursorsEqual !== void 0) return cursorsEqual;

return compare(current, next, _isImmutable, isEqualImmutable);
});
}

Expand Down Expand Up @@ -236,6 +184,22 @@ function factory (methods) {
}
}

function compare (current, next, typeCheck, equalCheck) {
var isCurrent = typeCheck(current);
var isNext = typeCheck(next);

if (isCurrent && isNext) {
return equalCheck(current, next);
}
if (isCurrent || isCurrent) {
return false;
}
return void 0;
}

function isEqualImmutable (a, b) {
return a === b;
}

/**
* Predicate to check if a potential is an immutable structure or not.
Expand All @@ -245,7 +209,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 +233,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 +244,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
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
49 changes: 42 additions & 7 deletions tests/shouldComponentUpdate-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,28 @@ describe('shouldComponentUpdate', function () {
});
});

it('when props has changed but not state', function () {
var data = Immutable.fromJS({ foo: 'bar', bar: [1, 2, 3] });
var state = { foo: 'hello' };
shouldUpdate({
cursor: { one: Cursor.from(data, ['foo']) },
nextCursor: { two: Cursor.from(data, ['foo']) },
state: state,
nextState: state
});
});

it('when state has changed but not props', function () {
var data = Immutable.fromJS({ foo: 'bar', bar: [1, 2, 3] });
var props = { one: Cursor.from(data, ['foo']) };
shouldUpdate({
cursor: props,
nextCursor: props,
state: { foo: 'hello' },
nextState: { foo: 'bar' }
});
});

it('when state has changed', function () {
shouldUpdate({
state: { foo: 'hello' },
Expand All @@ -88,14 +110,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 @@ -207,6 +229,19 @@ describe('shouldComponentUpdate', function () {
});
});

it('when neither props nor state has changed', function () {
var data = Immutable.fromJS({ foo: 'bar', bar: [1, 2, 3] });
var props = { one: Cursor.from(data, ['foo']) };
var state = { foo: 'hello' };
shouldNotUpdate({
cursor: props,
nextCursor: props,
state: state,
nextState: state
});
});


it('when passing same cursors', function () {
var data = Immutable.fromJS({ foo: 'bar' });

Expand Down Expand Up @@ -280,10 +315,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 +368,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

0 comments on commit 9f3715c

Please sign in to comment.