javascript - useEffect - Prevent infinite loop when updating state

I'd like the user to be able to sort a list of todo items. When the users selects an item from a dropdown it will set the sortKey which will create a new version of setSortedTodos, and in turn trigger the useEffect and call setSortedTodos.

The below example works exactly how I want, however eslint is prompting me to add todos to the useEffect dependancy array, and if I do it causes an infinite loop (as you would expect).

const [todos, setTodos] = useState([]);
const [sortKey, setSortKey] = useState('title');

const setSortedTodos = useCallback((data) => {
  const cloned = data.slice(0);

  const sorted = cloned.sort((a, b) => {
    const v1 = a[sortKey].toLowerCase();
    const v2 = b[sortKey].toLowerCase();

    if (v1 < v2) {
      return -1;
    }

    if (v1 > v2) {
      return 1;
    }

    return 0;
  });

  setTodos(sorted);
}, [sortKey]);

useEffect(() => {
    setSortedTodos(todos);
}, [setSortedTodos]);

Live Example:

const {useState, useCallback, useEffect} = React;

const exampleToDos = [
    {title: "This", priority: "1 - high", text: "Do this"},
    {title: "That", priority: "1 - high", text: "Do that"},
    {title: "The Other", priority: "2 - medium", text: "Do the other"},
];

function Example() {
    const [todos, setTodos] = useState(exampleToDos);
    const [sortKey, setSortKey] = useState('title');

    const setSortedTodos = useCallback((data) => {
      const cloned = data.slice(0);

      const sorted = cloned.sort((a, b) => {
        const v1 = a[sortKey].toLowerCase();
        const v2 = b[sortKey].toLowerCase();

        if (v1 < v2) {
          return -1;
        }

        if (v1 > v2) {
          return 1;
        }

        return 0;
      });

      setTodos(sorted);
    }, [sortKey]);

    useEffect(() => {
        setSortedTodos(todos);
    }, [setSortedTodos]);

    const sortByChange = useCallback(e => {
        setSortKey(e.target.value);
    });
    
    return (
        <div>
            Sort by:&nbsp;
            <select onChange={sortByChange}>
                <option selected={sortKey === "title"} value="title">Title</option>
                <option selected={sortKey === "priority"} value="priority">Priority</option>
            </select>
            {todos.map(({text, title, priority}) => (
                <div className="todo">
                    <h4>{title} <span className="priority">{priority}</span></h4>
                    <div>{text}</div>
                </div>
            ))}
        </div>
    );
}

ReactDOM.render(<Example />, document.getElementById("root"));

body {
    font-family: sans-serif;
}
.todo {
    border: 1px solid #eee;
    padding: 2px;
    margin: 4px;
}
.todo h4 {
    margin: 2px;
}
.priority {
    float: right;
}

<div id="root"></div>

<script src=".10.2/umd/react.production.min.js"></script>
<script src=".10.2/umd/react-dom.production.min.js"></script>

The below example works exactly how I want, however eslint is prompting me to add todos to the useEffect dependancy array, and if I do it causes an infinite loop (as you would expect).

const [todos, setTodos] = useState([]);
const [sortKey, setSortKey] = useState('title');

const setSortedTodos = useCallback((data) => {
  const cloned = data.slice(0);

  const sorted = cloned.sort((a, b) => {
    const v1 = a[sortKey].toLowerCase();
    const v2 = b[sortKey].toLowerCase();

    if (v1 < v2) {
      return -1;
    }

    if (v1 > v2) {
      return 1;
    }

    return 0;
  });

  setTodos(sorted);
}, [sortKey]);

useEffect(() => {
    setSortedTodos(todos);
}, [setSortedTodos]);

Live Example:

const {useState, useCallback, useEffect} = React;

const exampleToDos = [
    {title: "This", priority: "1 - high", text: "Do this"},
    {title: "That", priority: "1 - high", text: "Do that"},
    {title: "The Other", priority: "2 - medium", text: "Do the other"},
];

function Example() {
    const [todos, setTodos] = useState(exampleToDos);
    const [sortKey, setSortKey] = useState('title');

    const setSortedTodos = useCallback((data) => {
      const cloned = data.slice(0);

      const sorted = cloned.sort((a, b) => {
        const v1 = a[sortKey].toLowerCase();
        const v2 = b[sortKey].toLowerCase();

        if (v1 < v2) {
          return -1;
        }

        if (v1 > v2) {
          return 1;
        }

        return 0;
      });

      setTodos(sorted);
    }, [sortKey]);

    useEffect(() => {
        setSortedTodos(todos);
    }, [setSortedTodos]);

    const sortByChange = useCallback(e => {
        setSortKey(e.target.value);
    });
    
    return (
        <div>
            Sort by:&nbsp;
            <select onChange={sortByChange}>
                <option selected={sortKey === "title"} value="title">Title</option>
                <option selected={sortKey === "priority"} value="priority">Priority</option>
            </select>
            {todos.map(({text, title, priority}) => (
                <div className="todo">
                    <h4>{title} <span className="priority">{priority}</span></h4>
                    <div>{text}</div>
                </div>
            ))}
        </div>
    );
}

ReactDOM.render(<Example />, document.getElementById("root"));

body {
    font-family: sans-serif;
}
.todo {
    border: 1px solid #eee;
    padding: 2px;
    margin: 4px;
}
.todo h4 {
    margin: 2px;
}
.priority {
    float: right;
}

<div id="root"></div>

<script src="https://cdnjs.cloudflare./ajax/libs/react/16.10.2/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare./ajax/libs/react-dom/16.10.2/umd/react-dom.production.min.js"></script>

I'm thinking there has to be a better way of doing this that keeps eslint happy.

Share Improve this question edited Nov 25, 2019 at 9:17 T.J. Crowder 1.1m200 gold badges2k silver badges1.9k bronze badges asked Nov 25, 2019 at 8:43 DanV 3,2504 gold badges34 silver badges48 bronze badges

1 Just a side note: The sort callback can be just: return a[sortKey].toLowerCase().localeCompare(b[sortKey].toLowerCase()); which also has the advantage of doing a locale pare if the environment has reasonable locale information. If you like, you can throw destructuring at it, too: pastebin./7X4M1XTH – T.J. Crowder Commented Nov 25, 2019 at 8:48
What error is eslint throwing? – Luïs Commented Nov 25, 2019 at 8:50
Could you update the question to provide a runnable minimal reproducible example of the problem using Stack Snippets (the [<>] toolbar button)? Stack Snippets support React, including JSX; here's how to do one. That way people can check that their proposed solutions don't have the infinite loop problem... – T.J. Crowder Commented Nov 25, 2019 at 8:51
That's a really interesting approach, and a really interesting problem. As you say, you can understand why ESLint thinks you need to add todos to the dependency array on useEffect, and you can see why you shouldn't. :-) – T.J. Crowder Commented Nov 25, 2019 at 8:54
I added the live example for you. Really want to see this answered. – T.J. Crowder Commented Nov 25, 2019 at 9:09

| Show 1 more ment

3 Answers 3

Sorted by: Reset to default 8

I'd argue that this means that going about it this way is not ideal. The function is indeed dependent on todos. If setTodos is called somewhere else, the callback function has to be reputed, otherwise it operates on stale data.

Why do you store the sorted array in state anyway? You can use useMemo to sort the values when either the key or the array changes:

const sortedTodos = useMemo(() => {
  return Array.from(todos).sort((a, b) => {
    const v1 = a[sortKey].toLowerCase();
    const v2 = b[sortKey].toLowerCase();

    if (v1 < v2) {
      return -1;
    }

    if (v1 > v2) {
      return 1;
    }

    return 0;
  });
}, [sortKey, todos]);

Then reference sortedTodos everywhere.

Live Example:

const {useState, useCallback, useMemo} = React;

const exampleToDos = [
    {title: "This", priority: "1 - high", text: "Do this"},
    {title: "That", priority: "1 - high", text: "Do that"},
    {title: "The Other", priority: "2 - medium", text: "Do the other"},
];

function Example() {
    const [sortKey, setSortKey] = useState('title');
    const [todos, setTodos] = useState(exampleToDos);

    const sortedTodos = useMemo(() => {
      return Array.from(todos).sort((a, b) => {
        const v1 = a[sortKey].toLowerCase();
        const v2 = b[sortKey].toLowerCase();

        if (v1 < v2) {
          return -1;
        }

        if (v1 > v2) {
          return 1;
        }

        return 0;
      });
    }, [sortKey, todos]);

    const sortByChange = useCallback(e => {
        setSortKey(e.target.value);
    }, []);
    
    return (
        <div>
            Sort by:&nbsp;
            <select onChange={sortByChange}>
                <option selected={sortKey === "title"} value="title">Title</option>
                <option selected={sortKey === "priority"} value="priority">Priority</option>
            </select>
            {sortedTodos.map(({text, title, priority}) => (
                <div className="todo">
                    <h4>{title} <span className="priority">{priority}</span></h4>
                    <div>{text}</div>
                </div>
            ))}
        </div>
    );
}

ReactDOM.render(<Example />, document.getElementById("root"));

body {
    font-family: sans-serif;
}
.todo {
    border: 1px solid #eee;
    padding: 2px;
    margin: 4px;
}
.todo h4 {
    margin: 2px;
}
.priority {
    float: right;
}

<div id="root"></div>

<script src="https://cdnjs.cloudflare./ajax/libs/react/16.10.2/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare./ajax/libs/react-dom/16.10.2/umd/react-dom.production.min.js"></script>

There is no need to store the sorted values in state, since you can always derive/pute the sorted array from the "base" array and the sort key. I'd argue it also makes your code easier to understand since it is less plex.

The reason for the infinite loop is because todos doesn't match the previous reference and the effect will rerun.

Why use an effect for a click-action anyway? Your can run it in a function like so:

const [todos, setTodos] = useState([]);

function sortTodos(e) {
    const sortKey = e.target.value;
    const clonedTodos = [...todos];
    const sorted = clonedTodos.sort((a, b) => {
        return a[sortKey.toLowerCase()].localeCompare(b[sortKey.toLowerCase()]);
    });

    setTodos(sorted);
}

and on your dropdown do an onChange.

    <select onChange="sortTodos"> ......

Note on the dependency by the way, ESLint is right! Your Todos, in the case described above, are a dependency and should be in the list. The approach on the selection of an item is wrong, and hence your problem.

What you need to do here is to use functional form of setState:

  const [todos, setTodos] = useState(exampleToDos);
    const [sortKey, setSortKey] = useState('title');

    const setSortedTodos = useCallback((data) => {

      setTodos(currTodos => {
        return currTodos.sort((a, b) => {
          const v1 = a[sortKey].toLowerCase();
          const v2 = b[sortKey].toLowerCase();

          if (v1 < v2) {
            return -1;
          }

          if (v1 > v2) {
            return 1;
          }

          return 0;
        });
      })

    }, [sortKey]);

    useEffect(() => {
        setSortedTodos(todos);
    }, [setSortedTodos, todos]);

Working codesandbox

Even if you're copying the state in order to not mutate the original one, it's still not guaranteed that you'll get its latest value, due to setting state being async. Plus most of the methods will return a shallow copy, so you might end up mutating the original state anyways.

Using functional setState ensures that you get the latest value of the state and do not mutate the original state value.

科技改变生活-雨落星辰 - 所有的伟大,都源于一个勇敢的开始

javascript - useEffect - Prevent infinite loop when updating state - Stack Overflow

3 Answers 3

与本文相关的文章

评论列表(0)