reflux / refluxjs

A simple library for uni-directional dataflow application architecture with React extensions inspired by Flux
BSD 3-Clause "New" or "Revised" License
5.36k stars 330 forks source link

shouldComponentUpdate #516

Closed jonattanva closed 7 years ago

jonattanva commented 7 years ago

Hi, I have the following

Action

export default Reflux.createActions([ 'refresh',]);

Store

class ExampleStore extends Reflux.Store {
    state = {
       loading  : false,
       data : []
    }

    constructor() {
       super();
       this.listenToMany(Actions)
    }

    onRefresh() {
        this.setState({
            loading : true
        });

     request(/* request HTTP */).then(success => {
        this.setState({
            loading : false,
            data : success.data
        });
     })

    }
}

Component

class Example extends Reflux.Component {
    constructor(props) {
        super(props);
        this.store = ExampleStore;
    }

    shouldComponentUpdate(newProps, newState) {
       console.log(this.state.loading) // => true
       console.log(newState.loading) // => true
       console.log(this.state.data) // => []
       console.log(newState.data) // => [{...},{...}]
    }
}

How do I compare states if they are always the same?

BryanGrezeszak commented 7 years ago

Sorry, but I'm unable to replicate this at all.

I've tried basically your script:

var Actions = Reflux.createActions([ 'refresh',]);

class ExampleStore extends Reflux.Store {

    constructor() {
       super();
       this.state = {loading :false}
       this.listenToMany(Actions)
    }

    onRefresh() {
       this.setState({
           loading : true
       });

       // just turned this into arbitrary timeout...shouldn't change anything
       setTimeout(success => {
           this.setState({
               loading : false
           });
       }, 1000);
    }
}

class Example extends Reflux.Component {
    constructor(props) {
        super(props);
        this.store = ExampleStore;
    }

    shouldComponentUpdate(newProps, newState) {
       console.log(`old: ${this.state.loading}, new: ${newState.loading}`);
       return true;
    }

    render() {
       return React.createElement('div', null, `loading: ${this.state.loading}`);
    }
}

ReactDOM.render
(
    React.createElement(Example, null),
    document.querySelector('#react-root')
);

Actions.refresh();

And worked as expected, outputting:

> old: false, new: true
> old: true, new: false // <-- 1 sec later

Then I tried building my own simple example:

var Actions = Reflux.createActions(['increment']);

class Store extends Reflux.Store
{
    constructor()
    {
        super();
        this.state = {count:1};
        this.listenables = Actions;
    }

    onIncrement()
    {
        this.setState({count: this.state.count+1});
    }
}

class Root extends Reflux.Component
{
    constructor(props)
    {
        super(props);
        this.state = {};
        this.store = Store;
    }

    render()
    {
        return React.createElement('div', null, `count: ${this.state.count}`);
    }

    shouldComponentUpdate(newProps, newState)
    {
        console.log(`old: ${this.state.count}, new: ${newState.count}`);
        return true;
    }
}

ReactDOM.render
(
    React.createElement(Root, null),
    document.querySelector('#react-root')
);

Actions.increment();

And it also works completely as intended, outputting: old: 1, new: 2

BryanGrezeszak commented 7 years ago

Is there other state in that store that you are changing, and that is what is triggering your shouldComponentUpdate, and that the output is correct because loading actually didn't change on that particular state change?

That seems like the most likely case to me, as a first guess. If the only part of the state that the component is going to use is loading then maybe you would want to use this.storeKeys = ['loading'] in the component to restrict it to taking only the loading property from the store.

jonattanva commented 7 years ago

Sorry for not adding the complete code

import Reflux from 'reflux';
import React from 'react';
import ReactDOM from 'react-dom';

var Actions = Reflux.createActions([ 'refresh' ]);

class ExampleStore extends Reflux.Store {

    constructor() {
       super();
       this.state = {
           loading : {
               create : false,
               refresh : false
           }
       }
       this.listenToMany(Actions)
    }

    onRefresh() {
       this.setState({
           loading : Object.assign(this.state.loading, { refresh : true }) // This is the problem
       });

       // just turned this into arbitrary timeout...shouldn't change anything
       setTimeout(success => {
          this.setState({
              loading : Object.assign(this.state.loading, { refresh : false }) // This is the problem
           });
       }, 1000);
    }
}

class Example extends Reflux.Component {
    constructor(props) {
        super(props);
        this.store = ExampleStore;
    }

    shouldComponentUpdate(newProps, newState) {
        console.log(`create  -> old: ${this.state.loading.create}, new: ${newState.loading.create}`);
        console.log(`refresh -> old: ${this.state.loading.refresh}, new: ${newState.loading.refresh}`);
        return true;
    }

    componentDidMount() {
        Actions.refresh()
    }

    render() {
       return (
           <div>
               {`create: ${this.state.loading.create} - refresh: ${this.state.loading.refresh}`}
           </div>
       )
    }
}

ReactDOM.render(<Example />, document.getElementById('container'));
BryanGrezeszak commented 7 years ago

You're mutating the state directly because you're not using Object.assign quite right. Example:

var obj = {one:'1'};
Object.assign(obj, {two:'2'});
console.log(obj); // <- {one:'1', two:'2'}, obj itself was mutated, adding {two:'2'}

The first arg to Object.assign gets mutated. You never mutate state in react (you'd have the same problem if you were just doing that in a normal React.Component and doing nothing with Reflux at all).

To use Object.assign to avoid direct mutations you do like so:

var obj = {one:'1'};
var newObj = Object.assign({}, obj, {two:'2'}); // merge both into a new object
console.log(obj); // <- {one:'1'}, did not get mutated
console.log(newObj); // <- {one:'1', two:'2'}, new object without mutating old
jonattanva commented 7 years ago

Sorry for the loss of time. Thank you

BryanGrezeszak commented 7 years ago

No big deal, bud. Just makes a good search result for the next person with the same question :)