Question 219933 in Reactjs Question posted in
The new location for the React Documentation can be found here.

Reactjs – React derived state. Whats wrong with this code

Coming from angular, i would like to know what’s wrong with this code. I’ve been told that i should use useMemo (for the fitlering part) and useRef (to store products).
I know how to do that, but i want to better understand what’s wrong with this.
I have compared the number of renderings whenever searchTerm changes, and i see no difference.

export function ProductList({ productService }) {

  const [products, setProducts] = useState([])
  const [filteredProducts, setFilteredProducts] = useState([])

  const [searchTerm, setSearchTerm] = useState(null)

  function fetchData() {
    productService
      .allProducts()
      .then((products) => {
        setProducts(products)
        setFilteredProducts(products)
      })
      .catch(console.error)
  }

  function filterProductsByName(searchTerm) {
    setSearchTerm(searchTerm)
    setFilteredProducts(filterByName(products, searchTerm))
  }

  useEffect(fetchData, [])

  return (
    <div>
      <h1>Products</h1>
      <form onSubmit={(e) => e.preventDefault()}>
        <div>
          <label htmlFor="searchTerm">Search</label>
          <input
            type="text"
            name="searchTerm"
            id="searchTerm"
            onChange={(e) => filterProductsByName(e.target.value)}
          />
        </div>
      </form>
      <div>
        {filteredProducts.map((p) => (
          <Product key={p.id} product={p} />
        ))}
      </div>
    </div>
  )
}

I have compared both versions (with an without useMemo)

Tags:

2

Answers


  1. 0

    There nothing wrong with your code. But in case the filtering is rather expensive, you want to avoid it to be done too often – i.e. when the page gets rerendered for other reasons than a changed filter term. At the moment, when anything changes at the current page, the filtering will be re-applied. These changes are not even required in your current component to happen. Keep in mind how React rerenders:

    1. A state updates anywhere.
    2. React renders the whole page "virtually" with all its components. This means all calculation which aren’t excluded by their dependencies (by useEffect, useMemo, useCallback) will be executed again, even the expensive ones.
    3. React compares whether the virtual rerender results in a new DOM. In that case, a "real" rerendering in the brower happens.

    So,

    const filtered = useMemo(()=> filterByName(products, searchTerm), [products, searchTerm]);
...
setFilteredProducts(filtered);

    ensures the filtering won’t be run again as long as searchTerm remains the same, especially not when it’s just another component which receives a state update.

    But in practise, I wouldn’t over-optimize here. If the list to be filtered isn’t insanely long, there’s no need to introduce more complicated code until it harms performance. It’s even more error-prone with useMemo: in case you change your component at some time so it fetches new items not only initially but also time-based or based on an interaction and just adds them to the existing ones, a memorized value would suddenly bear wrong results. Long story short: avoid unnecessary complexity.

    Login or Signup to reply.
  2. 0

    I’ve been told that i should use useMemo (for the fitlering part) and useRef (to store products).

    Using useMemo to manage derived state is good advice, using a ref to store state … I’d question that decision. There are arguments to do this, but that’s a separate discussion.

    I have compared the number of renderings whenever searchTerm changes, and i see no difference.

    This ain’t an issue of performance but one where people can’t always keep track of all the possible states they may find themselves in.

    This is a fairly simple component, so it’s fairly simple to update the filteredProducts when you load the products and whenever the searchTerm changes, right? What about this:

    .then((products) => {
    setProducts(products)
    setFilteredProducts(products)
})

    when you load the products you set the filteredProducts to the same list. That implies that there is no searchTerm. So you assume that this request will happen so fast that nobody would have time to enter a searchTerm before you set the products … not even the browsers autocomplete? And what about a slow connection?

    Even in this simple code, you’ve already overlooked a possibility which may lead to filteredProducts not reflecting the searchTerm. It’s unlikely/rare, but possible.

    OK, then we’ll simply setFilteredProducts(filterByName(products, searchTerm)); and things are fine, right? But searchTerm in this context is a closure over the initial value, not its current value, so this doesn’t work either.

    Do you see the issue?

    The trivial solution would be to just do

    const filteredProducts = filterByName(products, searchTerm);

    but this may compute unnecessarily often, so we useMemo to only recompute that list when its dependencies change:

    const filteredProducts = useMemo(() => filterByName(products, searchTerm), [products, searchTerm]);

    that’s the "textbook solution";

    Then again, this is a pretty simple component. Tell me, what besides loading the products or changing the searchTerm would trigger a rerender of this component? Doesn’t that mean that you have to compute the filteredProducts on basically ever render anyways? useMemo is usually cheaper than the operation done within, but it ain’t free.

    I’d suggest you try

    function ProductList({ productService }) {

  const [products, setProducts] = useState([]);
  const [searchTerm, setSearchTerm] = useState(null);

  function fetchData() {
    productService
      .allProducts()
      .then(setProducts)
      .catch(console.error);
  }

  useEffect(fetchData, [productService]);

  return (
    <div>
      <h1>Products</h1>
      <form onSubmit={(e) => e.preventDefault()}>
        <div>
          <label htmlFor="searchTerm">Search</label>
          <input
            type="text"
            name="searchTerm"
            id="searchTerm"
            value={searchTerm}
            onChange={(e) => setSearchTerm(e.target.value)}
          />
        </div>
      </form>
      <div>
        {filterByName(products, searchTerm).map((p) => (
          <Product key={p.id} product={p} />
        ))}
      </div>
    </div>
  )
}

    One source of unnecessary rerenders may be the parent element. If that is the case you may want to consider React.memo().

    Also take a look on the topic of "controlled vs uncontrolled input" in react, maybe start here: React – Controlled Components.

    Login or Signup to reply.
Please signup or login to give your own answer.

Back To Top
Search