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.

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

Optimize for shouldComponentUpdate #1

Closed
gaperton opened this issue Mar 8, 2016 · 5 comments
Closed

Optimize for shouldComponentUpdate #1

gaperton opened this issue Mar 8, 2016 · 5 comments

Comments

@gaperton
Copy link

gaperton commented Mar 8, 2016

One of the problems with links is that they break pure render optimization.

An upcoming release of React contains deprecation message for their crappy valueLinks, which actually is the great opportunity to improve links design. We don't have to keep backward compatibility any more.

@gaperton
Copy link
Author

gaperton commented Mar 8, 2016

There are different design options.

  1. Link is the plain function. link() - get value, link( x ) - sets value.
getLink( name ){
    var model = this;
    return function link( x ){
         if( !arguments.length ) return model[ name ];

         model[ name ] = x;
    }
}

It's just one function with two variables closured. Not good, as combinators won't work without polluting the Function.prototype,

@gaperton
Copy link
Author

gaperton commented Mar 8, 2016

  1. Abstract class. It didn't worked with React links internals, now there won't be such a problem.
function Link(){}
Link.prototype = {
    set( x ){ throw new Error( "Not implemented" ); },
    get value(){ throw new Error( "Not implemented" ); }
}

So, in this case every object can implement specific links.

As for the model:

function ModelLink( model, attr ){
    this.model = model;
    this.attr = attr;

    // and... Yes! Pure Render.
    this._changeToken = model._changeToken;
}

ModelLink.prototype = {
    set( x ){ this.model[ this.attr ] = x; },
    get value(){ return this.model[ this.attr ]; }
}

Another option to support pure render for model links is to cache links in the model.

@gaperton
Copy link
Author

gaperton commented Mar 8, 2016

So, third caching option:

Model.prototype.getLink = function( attr ){
    var links = this._links || ( this._links = new this.Attributes() );
    return links[ attr ] || ( links[ attr ] = new Link( this, attr ) );
}

model._links should be removed on actual model change. This one should be quite efficient.

None of these things will actually help when top-level state is updated. For the links, we need to compare attribute value.

@gaperton gaperton changed the title Optimize of shouldComponentUpdate Optimize for shouldComponentUpdate Mar 8, 2016
@gaperton
Copy link
Author

gaperton commented Mar 8, 2016

So, comparing the attribute value. Good thing is that in case of presence of _changeToken, prop reference check is bypassed. Which allows us just to do like this:

ModelLink.prototype = {
    set( x ){ this.model[ this.attr ] = x; },
    get value(){ return this.model[ this.attr ]; },
    get _changeToken(){ return this.value; }
}

Some bugs are possible if model or attr name will be replaced, and value will be the same. This is quite rare case, but weird. I think, we could try to live with that. At least, it's easy to implement.

@gaperton
Copy link
Author

gaperton commented Mar 9, 2016

Remove any optimization by default. That's bullshit and is too dangerous. And not needed - the full power of links on the lower level is unleashed when we use stateless components as functions, where this optimization is not possible.

Also, in new design, it will be done in Link subclasses. In NestedReact. Which is right thing to do - it knows about both models and pureRender.

@gaperton gaperton closed this as completed Mar 9, 2016
gaperton pushed a commit that referenced this issue Jun 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant