8

I have a very simple component with a text field and a button:

<image>

It takes a list as input and allows the user to cycle through the list.

The component has the following code:

import * as React from "react";
import {Button} from "@material-ui/core";

interface Props {
    names: string[]
}
interface State {
    currentNameIndex: number
}

export class NameCarousel extends React.Component<Props, State> {

    constructor(props: Props) {
        super(props);
        this.state = { currentNameIndex: 0}
    }

    render() {
        const name = this.props.names[this.state.currentNameIndex].toUpperCase()
        return (
            <div>
                {name}
                <Button onClick={this.nextName.bind(this)}>Next</Button>
            </div>
        )
    }

    private nextName(): void {
        this.setState( (state, props) => {
            return {
                currentNameIndex: (state.currentNameIndex + 1) % props.names.length
            }
        })
    }
}

This component works great, except I have not handled the case when the state changes. When the state changes, I would like to reset the currentNameIndex to zero.

What is the best way to do this?


Options I have conciderred:

Using componentDidUpdate

This solution is ackward, because componentDidUpdate runs after render, so I need to add a clause in the render method to "do nothing" while the component is in an invalid state, if I am not careful, I can cause a null-pointer-exception.

I have included an implementation of this below.

Using getDerivedStateFromProps

The getDerivedStateFromProps method is static and the signature only gives you access to the current state and next props. This is a problem because you cannot tell if the props have changed. As a result, this forces you to copy the props into the state so that you can check if they are the same.

Making the component "fully controlled"

I don't want to do this. This component should privately own what the currently selected index is.

Making the component "fully uncontrolled with a key"

I am considering this approach, but don't like how it causes the parent to need to understand the implementation details of the child.

Link


Misc

I have spent a great deal of time reading You Probably Don't Need Derived State but am largely unhappy with the solutions proposed there.

I know that variations of this question have been asked multiple times, but I don't feel like any of the answers weigh the possible solutions. Some examples of duplicates:


Appendix

Solution using componetDidUpdate (see description above)

import * as React from "react";
import {Button} from "@material-ui/core";

interface Props {
    names: string[]
}
interface State {
    currentNameIndex: number
}

export class NameCarousel extends React.Component<Props, State> {

    constructor(props: Props) {
        super(props);
        this.state = { currentNameIndex: 0}
    }

    render() {

        if(this.state.currentNameIndex >= this.props.names.length){
            return "Cannot render the component - after compoonentDidUpdate runs, everything will be fixed"
        }

        const name = this.props.names[this.state.currentNameIndex].toUpperCase()
        return (
            <div>
                {name}
                <Button onClick={this.nextName.bind(this)}>Next</Button>
            </div>
        )
    }

    private nextName(): void {
        this.setState( (state, props) => {
            return {
                currentNameIndex: (state.currentNameIndex + 1) % props.names.length
            }
        })
    }

    componentDidUpdate(prevProps: Readonly<Props>, prevState: Readonly<State>): void {
        if(prevProps.names !== this.props.names){
            this.setState({
                currentNameIndex: 0
            })
        }
    }

}

Solution using getDerivedStateFromProps:

import * as React from "react";
import {Button} from "@material-ui/core";

interface Props {
    names: string[]
}
interface State {
    currentNameIndex: number
    copyOfProps?: Props
}

export class NameCarousel extends React.Component<Props, State> {

    constructor(props: Props) {
        super(props);
        this.state = { currentNameIndex: 0}
    }

    render() {

        const name = this.props.names[this.state.currentNameIndex].toUpperCase()
        return (
            <div>
                {name}
                <Button onClick={this.nextName.bind(this)}>Next</Button>
            </div>
        )
    }


    static getDerivedStateFromProps(props: Props, state: State): Partial<State> {

        if( state.copyOfProps && props.names !== state.copyOfProps.names){
            return {
                currentNameIndex: 0,
                copyOfProps: props
            }
        }

        return {
            copyOfProps: props
        }
    }

    private nextName(): void {
        this.setState( (state, props) => {
            return {
                currentNameIndex: (state.currentNameIndex + 1) % props.names.length
            }
        })
    }


}
sixtyfootersdude
  • 25,859
  • 43
  • 145
  • 213
  • Keep track of the relevant prop values in the state as well and use these to compare to the next props in the `getDerivedStateFromProps`. – Emile Bergeron Dec 19 '19 at 22:25
  • Does this answer your question? [Updating state on props change in React Form](https://stackoverflow.com/questions/32414308/updating-state-on-props-change-in-react-form) – Emile Bergeron Dec 19 '19 at 22:30
  • And exactly [the answer about `getDerivedStateFromProps`](https://stackoverflow.com/a/49868300/1218980) since the other answers are a little outdated. – Emile Bergeron Dec 19 '19 at 22:32
  • 3
    Why the component is getting the entire list of names when it's job is just displaying a single name? lift the state up and just pass the selected name as a prop and a function to update the `currentIndex`. When `names` are getting updated in the parent just reset `currentIndex` – Asaf Aviv Dec 19 '19 at 22:44
  • @AsafAviv - The job of the component is to cycle through names. Think of it as a `carousel` component or a name selector. – sixtyfootersdude Dec 19 '19 at 22:45
  • @EmileBergeron - I think that is currently my preferred solution but using `componentDidUpdate` is not completely terrible either. Both are kind of bad. – sixtyfootersdude Dec 19 '19 at 22:46
  • 1
    I like @AsafAvivs solution: pass a function `next` to the component. The button displays the name, and its onClick just calls the next function, located in the parent, which handles the cycling – Dániel Somogyi Dec 19 '19 at 22:46
  • @DánielSomogyi - I am going to think more about this. I was trying to delegate work to the component (the parent is already complex) but this is interesting... – sixtyfootersdude Dec 19 '19 at 22:48
  • Yea the `Carousel` component should hold the state of the carousel, the selected item should not care about any of this and follow the single responsibility principle which is in this case is just showing an item – Asaf Aviv Dec 19 '19 at 22:49
  • 1
    @DánielSomogyi - I think I will have a similar issue in the parent...... – sixtyfootersdude Dec 19 '19 at 22:53
  • @AsafAviv - I updated the code to make this more clear. – sixtyfootersdude Dec 19 '19 at 22:53
  • 1
    Can you include the parent component that holds the names? – Asaf Aviv Dec 19 '19 at 22:55
  • 1
    @DánielSomogyi that just moves the problem to the parent, **unless** the parent is the component doing the async fetching which could know when to reset the state. – Emile Bergeron Dec 19 '19 at 22:56
  • 1
    That's not an issue at all, the parent component sets the `names`, when it sets the `names` it also sets the `currentIndex` to 0 – Asaf Aviv Dec 19 '19 at 22:57
  • 1
    @AsafAviv it really depends on where the parent gets the `names` and if it makes sense for the parent to manage the logic of a carousel. – Emile Bergeron Dec 19 '19 at 23:01
  • 1
    @EmileBergeron Agree and that's why i asked to include the parent component to see if names are used in other components as well – Asaf Aviv Dec 19 '19 at 23:05
  • @EmileBergeron technically you are right, but i also think it makes sense to manage the currently shown name (or its index) in the same component, where the list of names get set. Of course that can blow up the parent(s), but you always can split a component in smaller parts... – Dániel Somogyi Dec 19 '19 at 23:06
  • I am going to echo @AsafAviv's suggestion as it is a well proven approach to handling state in the component + Daniel Somogyi's latest comment for managing the index. I will point out that this component should not care how it gets 'names' as long as it is provided with it from a parent/application level. Do not overcomplicate yourself, if you were to extend this functionality with multiple components to handle their own states/data structures and even add redux, you will appreciate the simplicity and power of this approach. – AGE Jan 06 '20 at 16:01

3 Answers3

4

As i said in the comments, i'm not a fan of these solutions.

Components should not care what the parent is doing or what is the current state of the parent, they should simply take in props and output some JSX, this way they are truly reusable, composable and isolated which also makes testing a lot easier.

We can make the NamesCarousel component hold the names of the carousel together with the functionality of the carousel and the current visible name and make a Name component which does only one thing, display the name that comes in through props

To reset the selectedIndex when the items are changing add a useEffect with items as a dependency, although if you just add items to the end of the array you can ignore this part

const Name = ({ name }) => <span>{name.toUpperCase()}</span>;

const NamesCarousel = ({ names }) => {
  const [selectedIndex, setSelectedIndex] = useState(0);

  useEffect(() => {
    setSelectedIndex(0)
  }, [names])// when names changes reset selectedIndex

  const next = () => {
    setSelectedIndex(prevIndex => prevIndex + 1);
  };

  const prev = () => {
    setSelectedIndex(prevIndex => prevIndex - 1);
  };

  return (
    <div>
      <button onClick={prev} disabled={selectedIndex === 0}>
        Prev
      </button>
      <Name name={names[selectedIndex]} />
      <button onClick={next} disabled={selectedIndex === names.length - 1}>
        Next
      </button>
    </div>
  );
};

Now this is fine but is the NamesCarousel reusable? no, the Name component is but the Carousel is coupled with the Name component.

So what can we do to make it truly reusable and see the benefits of designing component in isolation?

We can take advantage of the render props pattern.

Lets make a generic Carousel component which will take a generic list of items and invoke the children function passing in the selected item

const Carousel = ({ items, children }) => {
  const [selectedIndex, setSelectedIndex] = useState(0);

  useEffect(() => {
    setSelectedIndex(0)
  }, [items])// when items changes reset selectedIndex

  const next = () => {
    setSelectedIndex(prevIndex => prevIndex + 1);
  };

  const prev = () => {
    setSelectedIndex(prevIndex => prevIndex - 1);
  };

  return (
    <div>
      <button onClick={prev} disabled={selectedIndex === 0}>
        Prev
      </button>
      {children(items[selectedIndex])}
      <button onClick={next} disabled={selectedIndex === items.length - 1}>
        Next
      </button>
    </div>
  );
};

Now what this pattern actually gives us?

It gives us the ability to render the Carousel component like this

// items can be an array of any shape you like
// and the children of the component will be a function 
// that will return the select item
<Carousel items={["Hi", "There", "Buddy"]}>
  {name => <Name name={name} />} // You can render any component here
</Carousel>

Now they are both isolated and truly reusable, you can pass items as an array of images, videos, or even users.

You can take it further and give the carousel the number of items you want to display as props and invoke the child function with an array of items

return (
  <div>
    {children(items.slice(selectedIndex, selectedIndex + props.numOfItems))}
  </div>
)

// And now you will get an array of 2 names when you render the component
<Carousel items={["Hi", "There", "Buddy"]} numOfItems={2}>
  {names => names.map(name => <Name key={name} name={name} />)}
</Carousel>
Asaf Aviv
  • 11,279
  • 1
  • 28
  • 45
2

Can you use a functional component? Might simplify things a bit.

import React, { useState, useEffect } from "react";
import { Button } from "@material-ui/core";

interface Props {
    names: string[];
}

export const NameCarousel: React.FC<Props> = ({ names }) => {
  const [currentNameIndex, setCurrentNameIndex] = useState(0);

  const name = names[currentNameIndex].toUpperCase();

  useEffect(() => {
    setCurrentNameIndex(0);
  }, names);

  const handleButtonClick = () => {
    setCurrentIndex((currentNameIndex + 1) % names.length);
  }

  return (
    <div>
      {name}
      <Button onClick={handleButtonClick}>Next</Button>
    </div>
  )
};

useEffect is similar to componentDidUpdate where it will take an array of dependencies (state and prop variables) as the second argument. When those variables change, the function in the first argument is executed. Simple as that. You can do additional logic checks inside of the function body to set variables (e.g., setCurrentNameIndex).

Just be careful if you have a dependency in the second argument that gets changed inside the function, then you will have infinite rerenders.

Check out the useEffect docs, but you'll probably never want to use a class component again after getting used to hooks.

Tunn
  • 1,506
  • 16
  • 25
  • This is a good answer. Looks like it works due to the `useEffect`. If you update to explain why this works, I will mark this as the accepted answer. – sixtyfootersdude Jan 13 '20 at 16:51
  • Check out the updated answer, let me know if you need more clarification. – Tunn Jan 14 '20 at 02:48
1

You ask what is the best option, the best option is to make it a Controlled component.

The component is too low in the hierarchy to know how to handle it's properties changing - what if the list changed but only slightly (perhaps adding a new name) - the calling component might want to keep the original position.

In all cases I can think about we are better off if the parent component can decide how the component should behave when provided a new list.

It's also likely that such a component is part of a bigger whole and needs to pass the current selection to it's parent - perhaps as part of a form.

If you are really adamant on not making it a controlled component, there are other options:

  • Instead of an index you can keep the entire name (or an id component) in the state - and if that name no longer exists in the names list, return the first in the list. This is a slightly different behavior than your original requirements and might be a performance issue for a really really really long list, but it's very clean.
  • If you are ok with hooks, than useEffect as Asaf Aviv suggested is a very clean way to do it.
  • The "canonical" way to do it with classes seems to be getDerivedStateFromProps - and yes that means keeping a reference to the name list in the state and comparing it. It can look a bit better if you write it something like this:
   static getDerivedStateFromProps(props: Props, state: State = {}): Partial<State> {

       if( state.names !== props.names){
           return {
               currentNameIndex: 0,
               names: props.names
           }
       }

       return null; // you can return null to signify no change.
   }

(you should probably use state.names in the render method as well if you choose this route)

But really - controlled component is the way to go, you'll probably do it sooner or later anyway when demands change and the parent needs to know the selected item.

Alon Bar David
  • 1,757
  • 14
  • 15
  • While I disagree with: `The component is too low in the hierarchy to know how to handle it's properties changing`, this is the best answer. The alternatives your provide are interesting and useful. The primary reason I don't want to expose this prop is that this component will be used in multiple projects and I want the API to be minimal. – sixtyfootersdude Jan 13 '20 at 16:43
  • However one note: `If you are ok with hooks, than useEffect as Asaf Aviv suggested is a very clean way to do it.` - This does not solve the problem - despite having 4 votes, that question does not reset state on props-change. – sixtyfootersdude Jan 13 '20 at 16:44
  • `useEffect(() => { setSelectedIndex(0) }, [items]) ` UseEffect's second argument defines when it should run - so effectively it will run `setSelectedIndex(0)` whenever items change. Which is your requirement as far as I understand it. – Alon Bar David Jan 13 '20 at 17:15
  • @sixtyfootersdude the `useEffect` in my solution will reset the index when `names` changes, Tunn's `useEffect` is invalid and will show linting errors and will throw errors when names are changing. – Asaf Aviv Jan 13 '20 at 23:04
  • Worth mentioning that if it doesn't reset for you, you are mutating `names` directly which is another issue. – Asaf Aviv Jan 14 '20 at 02:00