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.

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

React mixin performance enhancement #35

Merged
merged 5 commits into from Jul 19, 2016

Conversation

Projects
None yet
2 participants
@burrows
Copy link
Member

burrows commented Jul 14, 2016

This PR updates the React mixins to ensure that forceUpdate is only called on a component once per flush cycle. This can lead to significant performance improvements in your React app where you have views that listen for changes to multiple properties that may change at the same time. Special care is also taken to avoid calling forceUpdate when a component has already been rendered within the same flush cycle by an ancestor component being updated.

In order to make sure that we properly handle property mutations made within an observer callback, the flush logic was also updated to only notify observers of property changes that occur prior to the flush starting. Any changes made within observers will be handled in a subsequent flush cycle. This should properly handle the case where the React mixin forces an update on a component, and then some other observer makes a property change that the component depends on. The component won't get re-rendered in that same flush cycle, but a new one will be queued up where that change will then be rendered.

burrows added some commits Jul 13, 2016

Change the implementation of flush to only process changes made before
flush was invoked. Any changes made by observers invoked by flush will
be handled in a subsequent flush cycle.
@burrows

This comment has been minimized.

Copy link
Member

burrows commented Jul 14, 2016

@peterwmwong @alexraginskiy Please take a look!

@@ -371,8 +382,8 @@ TransisObject.prototype.notify = function(event, ...args) {
//
// Returns the receiver.
TransisObject.prototype.didChange = function(name) {
(this.__changedProps__ = this.__changedProps__ || {})[name] = true;
changedObjects[this.objectId] = this;
changedObjects[this.objectId] = changedObjects[this.objectId] || {object: this, props: {}};

This comment has been minimized.

@peterwmwong

peterwmwong Jul 15, 2016

Contributor

Nit: Maybe consider registerChange function as this code is here and in propagateChanges().

function registerChange(object, prop){
  changedObjects[object.objectId] = changedObjects[object.objectId] || {objec, props: {}};
  changedObjects[object.objectId].props[prop] = true;
}

This comment has been minimized.

@burrows

burrows Jul 18, 2016

Member

Fixed in 991d76f.

@peterwmwong

This comment has been minimized.

Copy link
Contributor

peterwmwong commented Jul 15, 2016

LGTM


export var PropsMixin = function(props) {
return {
componentWillMount: function() {
this._transisFU = this._transisFU || (() => { this.isMounted() && this.forceUpdate(); });
this._transisId = this._transisId || nextId++;
this._transisQueueUpdate = this._transisQueueUpdate || (() => { queueUpdate(this); });

This comment has been minimized.

@peterwmwong

peterwmwong Jul 15, 2016

Contributor

I believe you can just define _transisQueueUpdate as part of the mixin, and it'll be prebound for you...

https://jsfiddle.net/hr01n87v/1/

This comment has been minimized.

@burrows

burrows Jul 15, 2016

Member

This would cause a problem if you included both mixins into the same component:

Uncaught Invariant Violation: ReactClassInterface: You are attempting to define `_doit` on your component more than once. This conflict may be due to a mixin.

@burrows burrows merged commit 369137d into master Jul 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment