cerebral-legacy / cerebral-view-react

React View layer package for Cerebral
16 stars 9 forks source link

Any child in shouldComponentUpdate returning false breaks the refresh #22

Closed bogusfocused closed 8 years ago

bogusfocused commented 8 years ago

//no decoration or no state. 
class Child extends React.Component {
shouldComponentUpdate ()
{
return false; //breaks parent refresh
}
}

@Cerebral()
class Parent extends React.Component
{
    render() {
   return <Child name={this.props.name}/>
}

Parent container fails to refresh its state if a child returns false.

bogusfocused commented 8 years ago

What I understand is the whole app is refreshed whenever an action is executed, so it really does not help to add state to intermediate hierarchy. A better way to be able to pass read only tree at root and let components pickup what they want or pass the values in props. The whole app refresh stuff is never pointed anywhere.

christianalfoni commented 8 years ago

Hi @rohitlodha,

Hm, I do not quite understand the scenario here. A child can not affect the rendering of its parent, only itself? When adding the Cerebral decorator a shouldComponentUpdate is added to the component checking whatever state is brought in from the state store and whatever props is passed form any parent component.

Is the situation that some component does not render when its supposed to?

bogusfocused commented 8 years ago

@christianalfoni Yep I have given up Baobab in favor of ImmutableJs. The way cerebral uses the model layer, baobab is not suitable. Baobab fire change, when interesting paths change and should be used to refresh UI unlike what Cerebral does. The way cerebral works, firing change without path information and using reference compares to detect changes, ImmutableJS fits very well. Baobab seems to have some trouble with monkeys.

christianalfoni commented 8 years ago

Ah, yes, I agree that ImmutableJs fits better with the whole "shallow check" strategy. That said, Baobab is immutable, so it works the same way. We just ignore the feature of listening to paths :-) You could say Baobab has a richer feature set allowing for different strategies, where Cerebral takes advantage of its immutability and ignoring the listening to paths feature.

Does that make sense?

bogusfocused commented 8 years ago

Object.freeze() is quite expensive. Does Baobab allow 'shallow checks' without Options.Immutability set true?

On Wed, Nov 11, 2015 at 3:08 PM, Christian Alfoni notifications@github.com wrote:

Ah, yes, I agree that ImmutableJs fits better with the whole "shallow check" strategy. That said, Baobab is immutable, so it works the same way. We just ignore the feature of listening to paths :-) You could say Baobab has a richer feature set allowing for different strategies, where Cerebral takes advantage of its immutability and ignoring the listening to paths feature.

Does that make sense?

— Reply to this email directly or view it on GitHub https://github.com/christianalfoni/cerebral-react/issues/22#issuecomment-155716688 .

christianalfoni commented 8 years ago

Yeah, immutableJS is faster indeed. Not sure at what size of tree and number of changes that it really matters though. Using Baobab on pretty large application now and we do not notice any issues. That said, ImmutableJS is faster, though more verbose in grabbing the values :)

Immutability is default and turning it off does not allow shallow checks anymore

SheikhG1900 commented 8 years ago

In mixin.js file, we have _update and listener functions. These functions call every time when state tree gets updated, cerebral doesn't care whether changes in state tree are for this component or for others. In order to save the cost of extra calling of these methods, should we listen for specific subtree changes instead of listening for whole tree, in all components?

christianalfoni commented 8 years ago

Hi there,

With immutability this is actually the common approach, but with the new version of Cerebral we are doing it differently. The new model packages will instead of giving the updated state on a change event, give an object describing the paths that have changed :) Video on it here: https://www.youtube.com/watch?v=xN7yg95AwXU

So this will improve a lot and give new debugger feature as well, video here: https://www.youtube.com/watch?v=o76KvYOKg90