reactjs / react.dev

The React documentation website
https://react.dev/
Creative Commons Attribution 4.0 International
11k stars 7.51k forks source link

Use a better example for Effect hook #1805

Open vjpr opened 5 years ago

vjpr commented 5 years ago

The following example is used in:

import React, { useState, useEffect } from 'react';

function Example() {
  const [count, setCount] = useState(0);

  // Similar to componentDidMount and componentDidUpdate:
  useEffect(() => {
    // Update the document title using the browser API
    document.title = `You clicked ${count} times`;
  });

  return (
    <div>
      <p>You clicked {count} times</p>
      <button onClick={() => setCount(count + 1)}>
        Click me
      </button>
    </div>
  );
}

This example creates confusion because the example doesn't actually need useEffect to operate correctly.

If you set document.title outside of the useEffect, the app behaves exactly the same.


Suggested changes

krzysztofpniak commented 5 years ago

I think the example is fine. It calls a really simple side effect (set title) when component is updated or mounted. Anyways, on the pages you mentioned there is more complicated example with chat events subscription. I think you missing the point a little bit. This effect is not designed to manipulate or access the DOM which is a bad practice in general. Of course you can do it, but main purpose is to call side effects, and set title is one of the simples one.

vjpr commented 5 years ago

There should be a note that says something like:

In this contrived example, if we didn't put the side-effect inside useEffect, it would still work exactly the same as if we did.

The only potential downside to not using useEffect is that the effect would be called more than it needs to, because it would be called on every React render (that is, when state, prop or context changes outside of this component).

We will see below that for a more complicated effect, there are more benefits to using useEffect.

gaearon commented 5 years ago

Open to concrete suggestions. I've thought about all these points when picking the example and yet couldn't find a better one that would carry the point across.

Zeko369 commented 4 years ago

@gaearon I know I'm a bit late to the party but hat do you think about data fetching on render or adding an event listener/setInterval, ChatAPI example isn't really clear IMO?

I know that this should be improved with error handling, but still fetching data seems more applicable than change document title for most people.

Simple fetcher example

const Component = () => {
  const [items, setItems] = useState([]);
  const [isLoading, setIsLoading] = useState(true);

  useEffect(() => {
    fetch(ITEMS_API)
      .then(data => data.json())
      .then(data => {
        setItems(data);
        setIsLoading(false);
      });
  }, []);

  return isLoading ? (
    <h1>Loading...</h1>
  ) : (
    <ul>
      {items.map(item => (
        <li key={item.id}>{item.title}</li>
      ))}
    </ul>
  );
};

Simple counter using setInteval

const Component = () => {
  const [count, setCount] = useState(0);

  useEffect(() => {
    const interval = setInterval(() => setCount(c => c + 1), 100);
    return () => clearInterval(interval);
  }, []);

  return <p>{count}</p>;
};
mhpreiman commented 3 years ago

I personally wish the subscription examples would be complete and actually runnable.

I also have an issue with Explanation: Why Effects Run on Each Update that says that without componentDidUpdate, "component would continue displaying the online status of a different friend. This is a bug." Yet I tried it out several times (changed friend.id with setState, and also as a component property <FriendStatusWithCounter friend={{id:1, isOnline:false}} />) and the componentDidUpdate had 0 effect on the id property (when I changed the value before re-render, the componentWillUnmount still received the original value). Grumble grumble..

eps1lon commented 3 years ago

This will be fixed by https://github.com/reactjs/reactjs.org/issues/3308