facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.13k stars 46.3k forks source link

Initial state from props #16461

Closed nachoaIvarez closed 5 years ago

nachoaIvarez commented 5 years ago

Do you want to request a feature or report a bug? Feature Request

What is the current behavior? Currently, the only way to reflect a prop change that serves as the initial state for the useState hook is via an explicit useEffect call.

const TodoList = ({ todos: initialTodos }) => {
  const [todos, setTodos] = useState(initialTodos);

  useEffect(() => setTodos(initialTodos), [initialTodos]);

  return (
    <ul>
      {todos.map(todo => (
        <li onClick={/* Do something that setTodos */}>{todo.title}</li>
      )}
    </ul>
  )
}

This is not particularly a rare use case. Like, displaying any filterable list in which the items need to be acted upon is basically a use case.

What is the expected behavior? The first thing that comes to mind, would be adding a second argument, which is, you guessed it, a dependency array.

const TodoList = ({ todos: initialTodos }) => {
  const [todos, setTodos] = useState(initialTodos, [initialTodos]);

  return (
    <ul>
      {todos.map(todo => (
        <li onClick={/* Something that setTodos */}>{todo.title}</li>
      )}
    </ul>
  )
}

There's probably a lot that I'm missing and there might be an obvious reason why this is not the actual behavior.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Latest.

markerikson commented 5 years ago

Seems like that would be fairly straightforward to extract as a custom hook?

threepointone commented 5 years ago

One of the problems with your example, is that todos ends up having 2 sources of truth; initialTodos via props, and the event handlers that change todos. What is the expected behaviour if TodoList renders with different initialTodos? (or maybe even, the same initialTodos, but with a different array instance because it was maybe cloned in the parent)

I would instead just remove the effect. Could you explain to me if I haven't understood, why you need to reflect the passed todos prop beyond the initial value?

nachoaIvarez commented 5 years ago

Thank you for your empathetic response, @threepointone.

Given your comment, it looks like I came up with a poor example to illustrate my issue and proposal. I can see my original comment may have implied there are two (unnecessary) sources of truth.

Here's, I think, a better example:

Imagine a view that lists files. Those files can be, let's say, renamed or rearranged via drag and drop client-side. Those changes are not persisted anywhere but the local state until user confirmation, after the user finished editing they confirm the changes by clicking a button that posts to an API.

At the same time, that same view, has some tabs on top to filter the files by their statuses. Selecting a tab sets a query string in the URL, which upon change triggers a fetch to an API that responds filtered files. Those filtered files you receive as props.

The prop serves as the initial state for the component, state that can be "discarded" upon prop change.

The only way I found to achieve this, was by doing

const FilesView = ({ files: initialFiles }) => {
  const [files, setFiles] = useState(initialFiles);
  useEffect(() => setFiles(initialFiles), [initialFiles]);

  return (
    // Render files that you can rename and drag-and-drop.
    // You can also filter them and receive the filtered results as prop.
  )
}

I think, cases like this may be the reason this section exists in the documentation for class-based components: https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#recommendation-fully-uncontrolled-component-with-a-key

Given the need exists and it was accounted for in the documentation for the "other" paradigm; in my opinion, having a dependency array as the second argument of useState would be a good solution, aligned with the rest of the built-in hooks, and (warning, this is strongly opinionated:) far more intuitive than using keys (and I don't even know how I would do it either, tbh)

nachoaIvarez commented 5 years ago

Just in case: I'm sharing all this with the intent of starting a conversation around something I came across using hooks multiple times. I suspect I'm not the only one (searched some questions on stackoverflow and kinda confirmed that's the case). And most importantly, I'm not by any means expecting any api changes as a random dude creating an issue

threepointone commented 5 years ago

Ok I think I understand your usecase now. There are 2 different recommendations

<FilesView key={window.location.href} todos={todos}/>

This will reset all state inside FilesView every time window.location changes (you could instead pass just a portion of your querystring). I think this is what you're looking for.

const FilesView = ({ files, setFiles }) => {
return ( // Render files that you can rename and drag-and-drop. // You can also filter them and receive the filtered results as prop. ) }

nachoaIvarez commented 5 years ago

For client-side apps, those two are good suggestions.

On SSR apps, I'd say your first recommendation can be achieved, but not without a bit of hacking around how to obtain that key. Would you agree it kinda smells the need of setting a URL/querystring, in a universal fashion, as key of a component in order to reset state? I think it's a bit "forced" and feels ad-hoc to me. My suggestion just follows the same pattern other hooks already have. I understand, though, the approach was the best we had before hooks.

The second approach I'd say is the good old container vs presentational components, no? In the context of a universal, non-react, static function providing props to components (like next.js, for instance) you couldn't do such thing, right?

In any case, would you agree that the docs could include some of these concepts in the hooks section? I'm suggesting this since in order to find this you need to be looking at the getDerivedStateFromProps section under the class-based components docs. Happy to help on this.

threepointone commented 5 years ago

It doesn't have to be the url, it just has to be a string that represents the current view (a function of the filtering arguments, in your case). Getting the url sounds simpler to me, but you're welcome to construct a string of your choosing.

For the second approach, I'd use context if passing props was harder.

And of course, if your'e comfortable with a hook, wrapping the functionality in a custom hook would make sense (though forewarned as I originally mentioned, it'll be buggy and represent 2 sources of truth for state).

Maybe the docs could use something that made this clearer, but that's a separate topic ofc. I'm unsure what that would look like. Maybe some SEO juice for "initial state from props"; it seems to come up often enough.

I'm going to close this issue since it isn't technically a bug or feature request anymore. I hope you're unblocked!