Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Make shouldComponentUpdate optimalization optional #18

Closed
rafal87 opened this issue Jan 21, 2016 · 3 comments
Closed

Make shouldComponentUpdate optimalization optional #18

rafal87 opened this issue Jan 21, 2016 · 3 comments

Comments

@rafal87
Copy link

rafal87 commented Jan 21, 2016

There is a known issue facebook/react#2517 when shouldComponentUpdate prevents rerendering children components depended on the context. It would be nice to make this optimalization optional for compatiblitiy with non reactive libs e.g react-router and it's Link component (applying active link style depends on the context).

@mweststrate
Copy link
Member

Hmm hard to form an opinion about it as the best practice seems to be in flux about this currently? Especially since they seem to want to move away from context in React-Router if I understand it correctly? Note that you can still implement your own shouldComponentUpdate.

But if there is no convenient way around this issue I can add an option for it. Do you have a suggested syntax?

@mweststrate
Copy link
Member

OK, I took a closer look a the facebook issue and currently there seems now way to cleanly handle it, as it is not possible to detect context changes on components that don't use the context themselves, so shouldComponentUpdate cannot act properly on it out of the box. This all depends on facebook/react#3973.

As mentioned in the original issue react-router is moving away from this constructions so I think we cannot do much more than wait for either one of the two earlier things happen.


You could define your own shouldComponentUpdate on all the intermediate components to just return false, but this will by-pass a lot of optimizations, especially; whenever a parent is re-rendered all of the children will be re rendered as well. That should work fine for medium sized apps but I am a bit reluctant to provide such a possibility out of the box in the api while react and react-router are in flux concerning this issue.

I think in general the only clean solution to this now is make context objects singletons to which one can subscribe manually as suggested by Dan Abramov in the beginning of the thread.

@rafal87
Copy link
Author

rafal87 commented Jan 29, 2016

As a temporary solution I monkey patched observer function in such way that it adds a shouldComponentUpdate method returning true if it's not already defined on components class. Code below is ran before any other module requires observer. Note that it does not change observer behavior on components definied as pure functions but in my case it is ok.

let observer = require('mobservable-react').observer;
let React = require('react');
require('mobservable-react').observer = function (componentClass) {
    if (componentClass.prototype.render) {
        let target = componentClass.prototype || componentClass;
        if (!target.shouldComponentUpdate) {
            target.shouldComponentUpdate = () => true;
        }
    }
    return observer(componentClass);
};

In my specific issue problem was not limited to react-router but also affected some custom components. In the long term refactoring them to observers should work better than depending on context modification. I must investigate react-router solution you mentioned.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants