2

I have infinite slideshow on my project, I'm not sure if my methods are in right order. I'm calling fetch function on componentWillMount() and then using that data on componentDidMount()..

The problem could be something else but it worked before, But now it doesn't..

      componentWillMount() {
    this.props.getAdverts();
  }

  componentDidMount() {
      var index = 0;
      setInterval(() => {
        this.setState({
          bg:  this.props.adverts ? this.props.adverts[index++].photo: this.state.bg,
          text:  this.props.adverts ? this.props.adverts[index++].text: this.state.text
          })
          if(index === this.props.adverts.length) index = 0;
        }, 4000) 
      }

When I log the this.props.adverts, it's array..

The error is: Cannot read property 'photo' of undefined or Cannot read property 'text' of undefined

STACKBLITZ link: https://stackblitz.com/edit/react-sshop

Randy Casburn
  • 13,840
  • 1
  • 16
  • 31
Merim
  • 1,283
  • 2
  • 21
  • 36
  • 1
    You are not using redux? Maybe it's better to make getAdverts return a promise and when it resolves just do a `this.setState` with the result. – HMR May 12 '18 at 15:14
  • I'm using redux but didn't show everything here, I'm going to put Stackblitz link in post, forgot it.. – Merim May 12 '18 at 15:15
  • 1
    You're not meant to fetch in `componentWillMount`, should be in `componentDidMount`. Either set up your components so they aren't rendered until the adverts have been received from the fetch, or that the components handle not having the prop to start with (like a flag in state and rendering a `loading` message if the advert prop isn't loaded yet, for example) – Jayce444 May 12 '18 at 15:31
  • A quick look at the project looks fine. I assume this is productList but the fetch fails on stackblitz. I don't think there is a need for any of the code in `componentDidMount` since `getAdverts` will produce an action (GET_PRODUCTS_FAILED in my case) that will set the state of the application. – HMR May 12 '18 at 15:37
  • If I set just photo without text in component state, it works, strange.. – Merim May 12 '18 at 16:18
  • It might be because the data comes in late and when you're calling `his.props.adverts` it might not be populated. So try putting the setState inside an `if` statement. `if(this.props.advert.length > 0) { ` – illiteratewriter May 12 '18 at 16:38
  • Because `index++` is the same as `index = index + 1`, `bg` is incrementing `1` and then `text` is incrementing again. Also remove `componentWillMount` and I suggest making `getAdverts` a `Promise` that you call in `componentDidMount`. Also, store the returned ref from `setInterval` and in `componentWillUnmount` send it into `clearInterval` or that counter will leak. – Victoria French May 12 '18 at 23:41

1 Answers1

2

This is an example of how you might want to do this using your current code. I disagree with the way this is being done, but for starters this should help you to start exploring a more React-ish way of coding.

// This is a composable function, we will pass it in to setInterval.
// we need access to the component, so we will pass it in and then
// return the function signature that setInterval wants to call
const changeAdvertComposer = function changeAdvertComposer(component) {
     // we start at -1 so the first call asks for 0
     let index = -1;

     // return the function that setInterval will call
    return function changeAdvert() {
          const adverts = component.props.adverts;
          if (!adverts || !adverts.length) {
               // we have no advertisements so lets exit
               return;
          }
          index++;
          // reset our index if needed
          if (index >= adverts.length) {
              index = 0;
          }
          // increment and grab our object
          const advert = adverts[index];

          // grab our state or a default failover structure
          const state = component.state.advert || { bg: '', text: '' };
          // set our state
          component.setState({
              advert: {
                   bg: advert.photo || state.bg,
                   text: advert.text || state.text,
              }
          });
     }
};

export ExampleAdvertManager extends Component {

    // setup some validations on our props
    static propTypes = {
         getAdverts: PropTypes.func.isRequired,
         adverts: PropTypes.arrayOf(PropTypes.shape({
              photo: PropTypes.string,
              text: PropTypes.string
         }))
    }

    constructor(props) {
         super(props);

         // we will store the state in the interval object so we can
         // check for null (simple way to find out if loading)
         this.state = {
              advert: null
         };

         // we will store the ref to our interval here
         this._intervalRef = null;
    }

    componentDidMount() {
         // we are loaded let's call our data action loader
         this.props.getAdverts();
    }

    componentWillUpdate() {
        // do we have any ads?
        const adlength = this.props.adverts ? this.props.adverts.length : 0;

        if (adlength && !this._intervalRef) {
             // we have ads and we have not setup the interval so lets do it
             this._intervalRef = setInterval(changeAdvertComposer(this), 4000);
        } else if (!adlength && this._intervalRef) {
             // we have no ads but we have an interval so lets stop it
             clearInterval(this._intervalRef);
             this._intervalRef = null;
        }
    }

    componentWillUnmount() {
         // we are unloading, lets clear up the interval so we don't leak
         if (this._intervalRef) {
               clearInterval(this._intervalRef);
               this._intervalRef = null;
         }
    }

    render() {
        if (this.stage.advert) {
            // render here
        }
        // we don't have data yet
        return null; // or some loading view
    }
}

And I may have gone overboard in this example, I've been doing this too long and really depend on composability for unit testing. It makes it hard for me to not think in that manner. I did not make setState composable though... the rabbit hole goes much deeper.

Really I would have made the interval timer a component of its own that renders null and fires callbacks to my component. Just makes everything cleaner. That would look something like this:

class TimerComponent extends PureComponent {
      static propTypes = {
           onInterval: PropTypes.func.isRequired,
           interval: PropTypes.number.isRequired,
           immediate: PropTypes.bool,
      }

      static defaultProps = {
            immediate: true,
      }

      componentDidMount() {
          this._intervalRef = setInterval(this.props.onInterval, this.props.interval);
          if (this.props.immediate) {
               this.props.onInterval();
          }
      }

      componentWillUnmount() {
          clearInterval(this._intervalRef);
      }

      render() {
          return null;
      }
}

// We will still use the composable function, but in a differnt way.
// The function stays the same
const changeAdvertComposer = function changeAdvertComposer(component) {
     // we start at -1 so the first call asks for 0
     let index = -1;

     // return the function that setInterval will call
    return function changeAdvert() {
          const adverts = component.props.adverts;
          if (!adverts || !adverts.length) {
               // we have no advertisements so lets exit
               return;
          }
          index++;
          // reset our index if needed
          if (index >= adverts.length) {
              index = 0;
          }
          // increment and grab our object
          const advert = adverts[index];

          // grab our state or a default failover structure
          const state = component.state.advert || { bg: '', text: '' };
          // set our state
          component.setState({
              advert: {
                   bg: advert.photo || state.bg,
                   text: advert.text || state.text,
              }
          });
     }
};

export ExampleAdvertManager extends Component {

    // setup some validations on our props
    static propTypes = {
         getAdverts: PropTypes.func.isRequired,
         adverts: PropTypes.arrayOf(PropTypes.shape({
              photo: PropTypes.string,
              text: PropTypes.string
         }))
    }

    constructor(props) {
         super(props);

         // we will store the state in the interval object so we can
         // check for null (simple way to find out if loading)
         this.state = {
              advert: null
         };
    }

    componentDidMount() {
         // we are loaded let's call our data action loader
         this.props.getAdverts();
    }

    render() {
        if (this.stage.advert) {
            return (
                <div>
                    <TimerComponent interval={4000} onInterval={changeAdvertComposer(this)} />
                    {
                       // render what you want to do from state
                    }
                </div>
            );
        } else if (this.props.adverts) {
            return (
                 <TimerComponent key="interval" interval={4000} onInterval={changeAdvertComposer(this)} />
            );
        }
        // we don't have data yet
        return null; // or some loading view
    }
}

I hope this helps.

Code Example

Running Demo

Victoria French
  • 756
  • 5
  • 10
  • It really, really helps, thank you for your effort and these comments, like to see why is something there. :) – Merim May 13 '18 at 04:36