Skip to content

Added support of shouldComponentUpdate #39

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

Open
medyo wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Added support of shouldComponentUpdate #39

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e7d97fd
Added support of shouldComponentUpdate
medyo May 10, 2015
00d68b5
added shouldComponentUpdate documentation
medyo May 12, 2015
c5d7301
fixed Trigger Update
medyo May 12, 2015
fbbe8e4
Fixed tests and add shouldComponentUpdate tests
medyo May 12, 2015
2c4287d
fixed a little mistake
medyo May 12, 2015
ddfab83
fixed a little mistake
medyo May 12, 2015
e693bd5
removed an unnecessary verification
medyo May 12, 2015
05ee271
added new test for forceUpdate
medyo May 12, 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
16 changes: 16 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,22 @@ React.BackboneMixin({
});
```

If in some cases you need to control whatever a change should re-render the view or not:
```javascript
var UserViewComponent = React.createBackboneClass({
shouldComponentUpdate: function() {
return this.getModel().previous('name') != this.getModel().get('name');
},
render: function() {
return (
<div>
<h1>{this.getModel().get("name")}</h1>
</div>
);
}
});
```

If your Collection or Model class does not inherit directly from Backbone.Model
or Backbone.Collection, you may customize the behavior on a library level by
overriding the React.BackboneMixin.ConsiderAsCollection function.
Expand Down
99 changes: 79 additions & 20 deletions __tests__/react.backbone-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,23 @@ describe('react.backbone', function() {

it('renders model as-is', function() {
var user = new Backbone.Model({name: "Clay"});
var userViewRef = UserView({model: user});
var userViewRef = React.createFactory(UserView)({model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);

var header = TestUtils.findRenderedDOMComponentWithTag(userView, 'h1');
expect(header.getDOMNode().textContent).toEqual('Clay');
});

it('renders model after changes to property', function() {
var user = new Backbone.Model({name: "Clay"});
var userViewRef = UserView({model: user});
var userViewRef = React.createFactory(UserView)({model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);
user.set("name", "David");

var header = TestUtils.findRenderedDOMComponentWithTag(userView, 'h1');
expect(header.getDOMNode().textContent).toEqual('David');
});

describe("with changeOptions", function() {
var UserView = React.createBackboneClass({
changeOptions: "change:name",
Expand All @@ -51,7 +51,7 @@ describe('react.backbone', function() {

it('doesnt render if other property is changed', function() {
var user = new Backbone.Model({name: "Clay", age: "80"});
var userViewRef = UserView({model: user});
var userViewRef = React.createFactory(UserView)({model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);
user.set("age", "60");

Expand All @@ -61,7 +61,7 @@ describe('react.backbone', function() {

it('does render if valid property is changed', function() {
var user = new Backbone.Model({name: "Clay", age: "80"});
var userViewRef = UserView({model: user});
var userViewRef = React.createFactory(UserView)({model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);
user.set("name", "David");

Expand All @@ -70,13 +70,38 @@ describe('react.backbone', function() {
});

});

describe("with shouldComponentUpdate", function() {
it('doesnt auto-update if shouldComponentUpdate is false', function() {
var UserView = React.createBackboneClass({
shouldComponentUpdate: function(){
return this.getModel().previous('name') != this.getModel().get('name');
},
render: function() {
return (
<div>
<h1>{this.getModel().get("name")} {this.getModel().get("age")}</h1>
</div>
);
}
});
var user = new Backbone.Model({name: "Mehdi", age: "80"});
var userViewRef = React.createFactory(UserView)({model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);
user.set("age", "100");

var header = TestUtils.findRenderedDOMComponentWithTag(userView, 'h1');
expect(header.getDOMNode().textContent).toEqual('Mehdi 80');
});

});
});

describe("with :collection key", function() {
var UsersListView = React.createBackboneClass({
render: function() {
var usersList = this.getCollection().map(function(user) {
return <li>{user.get("name")}</li>;
var usersList = this.getCollection().map(function(user, index) {
return <li key={index} >{user.get("name")}</li>;
});

return (
Expand All @@ -89,7 +114,7 @@ describe('react.backbone', function() {

it('renders collection as-is', function() {
var usersList = new Backbone.Collection([{name: "Clay"}, {name: "David"}]);
var usersListViewRef = UsersListView({collection: usersList});
var usersListViewRef = React.createFactory(UsersListView)({collection: usersList});
var usersListView = TestUtils.renderIntoDocument(usersListViewRef);

jest.runOnlyPendingTimers();
Expand All @@ -102,7 +127,7 @@ describe('react.backbone', function() {
it('renders collection on adding', function() {

var usersList = new Backbone.Collection([{name: "Clay"}, {name: "David"}]);
var usersListViewRef = UsersListView({collection: usersList});
var usersListViewRef = React.createFactory(UsersListView)({collection: usersList});
var usersListView = TestUtils.renderIntoDocument(usersListViewRef);
usersList.add({name: "Jack"});

Expand All @@ -112,6 +137,41 @@ describe('react.backbone', function() {
expect(list.getDOMNode().childNodes.length).toEqual(3);
expect(list.getDOMNode().childNodes[2].textContent).toEqual("Jack");
});

describe("with shouldComponentUpdate", function() {
it('doesnt auto-update if shouldComponentUpdate is false', function() {

var UsersListView = React.createBackboneClass({
shouldComponentUpdate: function(nextProps, nextState){
return false;
},
render: function() {
var usersList = this.getCollection().map(function(user, index) {
return <li key={index} >{user.get("name")}</li>;
});

return (
<ul>
{ usersList }
</ul>
);
}
});

var usersList = new Backbone.Collection([{name: "Mehdi"}, {name: "David"}]);
var usersListViewRef = React.createFactory(UsersListView)({collection: usersList});
var usersListView = TestUtils.renderIntoDocument(usersListViewRef);
usersList.add({name: "Jack"});

jest.runOnlyPendingTimers();

var list = TestUtils.findRenderedDOMComponentWithTag(usersListView, 'ul');
expect(list.getDOMNode().childNodes.length).toEqual(2);
expect(list.getDOMNode().childNodes[2]).toEqual(null);

});

});
});

describe("with mixins", function() {
Expand All @@ -133,7 +193,7 @@ describe('react.backbone', function() {
it("should render mixins as-is", function() {
var user = new Backbone.Model({name: "Clay"});
var wall = new Backbone.Model({post_count: 5});
var profileViewRef = ProfileView({user: user, wall: wall});
var profileViewRef = React.createFactory(ProfileView)({user: user, wall: wall});
var profileView = TestUtils.renderIntoDocument(profileViewRef);

var header = TestUtils.findRenderedDOMComponentWithTag(profileView, 'h1');
Expand All @@ -147,7 +207,7 @@ describe('react.backbone', function() {
it("should re-render if either mixin model is changed", function() {
var user = new Backbone.Model({name: "Clay"});
var wall = new Backbone.Model({post_count: 5});
var profileViewRef = ProfileView({user: user, wall: wall});
var profileViewRef = React.createFactory(ProfileView)({user: user, wall: wall});
var profileView = TestUtils.renderIntoDocument(profileViewRef);

user.set("name", "David");
Expand Down Expand Up @@ -177,7 +237,7 @@ describe('react.backbone', function() {
});
it("should use that prop", function() {
var user = new Backbone.Model({name: "Clay"});
var userViewRef = UserView({user_model: user});
var userViewRef = React.createFactory(UserView)({user_model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);

var header = TestUtils.findRenderedDOMComponentWithTag(userView, 'h1');
Expand All @@ -203,7 +263,7 @@ describe('react.backbone', function() {
});
it("should use that event", function() {
var user = new Backbone.Model({name: "Clay", age: 25});
var userViewRef = UserView({user_model: user});
var userViewRef = React.createFactory(UserView)({user_model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);
var header;

Expand Down Expand Up @@ -235,7 +295,7 @@ describe('react.backbone', function() {
});
it("should use that return value", function() {
var user = new Backbone.Model({name: "Clay"});
var userViewRef = UserView({user_model: user});
var userViewRef = React.createFactory(UserView)({user_model: user});
var userView = TestUtils.renderIntoDocument(userViewRef);

var header = TestUtils.findRenderedDOMComponentWithTag(userView, 'h1');
Expand Down Expand Up @@ -268,7 +328,7 @@ describe('react.backbone', function() {
expect(object).toEqual(users);
return true;
};
var userViewRef = UserView({ collection: users });
var userViewRef = React.createFactory(UserView)({collection: users});
TestUtils.renderIntoDocument(userViewRef);
});
it("should pass model to config function for test", function () {
Expand All @@ -278,7 +338,7 @@ describe('react.backbone', function() {
expect(object).toEqual(user);
return false;
};
var userViewRef = UserView({ model: user });
var userViewRef = React.createFactory(UserView)({model: user});
TestUtils.renderIntoDocument(userViewRef);
});

Expand All @@ -296,7 +356,7 @@ describe('react.backbone', function() {
React.BackboneMixin.ConsiderAsCollection = function (object) {
return false;
};
var userViewRef = UserView({ model: usersModel });
var userViewRef = React.createFactory(UserView)({model: usersModel});
TestUtils.renderIntoDocument(userViewRef);
});

Expand All @@ -306,13 +366,12 @@ describe('react.backbone', function() {
React.BackboneMixin.ConsiderAsCollection = function (object) {
return true;
};
var userViewRef = UserView({ collection: usersCollection });
var userViewRef = React.createFactory(UserView)({collection: usersCollection});
TestUtils.renderIntoDocument(userViewRef);
});

});

});
});

});
12 changes: 10 additions & 2 deletions react.backbone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@
updateScheduler: _.identity
};

var updatable = function (component){
if (component.shouldComponentUpdate !== undefined && component.shouldComponentUpdate !== null){
Copy link
Copy Markdown
Collaborator

@markijbema markijbema May 12, 2015

Choose a reason for hiding this comment

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

is the undefined test really necesarry? I would expect react to make a default one if it isn't defined. Did you check this?

Copy link
Copy Markdown
Contributor

@tsjoberg tsjoberg May 12, 2015

Choose a reason for hiding this comment

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

added line note below on the same lines to call shouldComponentUpdate() directly in the triggerUpdate

Copy link
Copy Markdown
Author

@medyo medyo May 12, 2015

Choose a reason for hiding this comment

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

the undefined is not really necessary, I am accustomed to check both cases undefined and null

return component.shouldComponentUpdate();
}
return true;
}

var subscribe = function(component, modelOrCollection, customChangeOptions) {
if (!modelOrCollection) {
return;
Expand All @@ -33,7 +40,7 @@
var behavior = React.BackboneMixin.ConsiderAsCollection(modelOrCollection) ? collectionBehavior : modelBehavior;

var triggerUpdate = behavior.updateScheduler(function() {
if (component.isMounted()) {
if (component.isMounted() && updatable(component)) {
Copy link
Copy Markdown
Contributor

@tsjoberg tsjoberg May 12, 2015

Choose a reason for hiding this comment

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

Couldn't you just add component.shouldComponentUpdate() here instead of calling helper function?

Copy link
Copy Markdown
Author

@medyo medyo May 12, 2015

Choose a reason for hiding this comment

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

shouldComponentUpdate isn't always defined, for that reason i've added updatable function to handle condition there.

Copy link
Copy Markdown
Contributor

@tsjoberg tsjoberg May 12, 2015

Choose a reason for hiding this comment

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

Ok yes makes sense - strange that React doesn't have a default implementation for shouldComponentUpdate{return true;} and having the spec override the implementation (like other lifecycle methods). But looks like most implementations on React are like this and not Backbone esq where you override default methods with your own etc.

(component.onModelChange || component.forceUpdate).call(component);
}
});
Expand Down Expand Up @@ -79,7 +86,7 @@

unsubscribe(this, modelOrCollection(this.props));
subscribe(this, modelOrCollection(nextProps), customChangeOptions);

if (typeof this.componentWillChangeModel === 'function') {
this.componentWillChangeModel();
}
Expand All @@ -98,6 +105,7 @@
componentWillUnmount: function() {
// Ensure that we clean up any dangling references when the component is destroyed.
unsubscribe(this, modelOrCollection(this.props));

}
};
};
Expand Down