useEffect for synchronizing state and props

[Yakimych]

Hi. I have a recurring scenario that I’ve been struggling with since the good old days of componentWillReceiveProps, and now I’ve pretty much run into the same issue with hooks, so I was hoping I could get some guidance as to what the idiomatic way of solving this and similar cases in React is.

Problem description - starting point

I have a list of items. Every item has an Edit button next to it. Clicking it opens an “Editor”, where one can change all the fields and either Confirm or Cancel. (Confirming would send an API call to save the data, but this part is not relevant to the problem I am having.) The “parent” component would render the list with the Edit buttons, and have an itemUnderEdit property that would be null from the start. Clicking on “Edit” for a specific item would set the itemUnderEdit to the clicked item.

Here is the full example with all 3 solutions on CodeSandbox: https://codesandbox.io/s/2oz2nzynpy

Solution 1

Make the “Editor” component stateless and controlled - it takes in change handlers for every field as props with the parent tracking every change. This solution appeals to me, since I like pure stateful components that are a one-to-one mapping of props to HTML - they are simple to reason about etc etc. This kind of goes against the commonly heard “keep your state close to where it is used” advice, which also seems reasonable, since I don’t really need to know in the parent what the user is typing, I am only interested to know when they are done at the end. This stateless solution also introduces a lot of props, since I need one event handler per field (onNameChanged, onDescriptionChanged in the example, but it could as well be 10 fields), which is a lot of props.

Solution 2

Make the “Editor” component stateful and only get an event when editing is done: onConfirm(itemToSave) or onCancel(). This seems like the “React” way and is in line with the advice of keeping state close to where it is used. Since I am only interested to know when the user clicks Confirm, a stateful “blackbox”-component that tracks its own state seems reasonable.

In order to achieve this, however, I need to copy my props to the state, which, according to @gaearon, is a bad idea:

const [name, setName] = useState(props.item.name);
const [description, setDescription] = useState(props.item.description);

Moreover, this solution is buggy from the start, since clicking on Edit for a different item doesn’t “re-sync” the props with the state - it only works if I close the Editor and then reopen it:

Which brings us to Solution 3.

Solution 3

This one has been one of my biggest pain-points with stateful components in React (which is why I prefer stateless components with a state container, but those I widely demonized nowadays, so I am yet again trying to understand the idiomatic React way of doing this).
The “old” ways were to sync in componentWillReceiveProps and later with getDerivedStateFromProps. Now I can do this with useEffect, where I specify props.item as the “dependency”, since I want to run it when the item changes.

useEffect(() => {
  if (props.item.name !== name) {
    setName(props.item.name);
  }
  if (props.item.description !== description) {
    setDescription(props.item.description);
  }
}, [props.item]);

This seems to work as expected, but I get the linter warning: React Hook useEffect has missing dependencies: 'description' and 'name'. Either include them or remove the dependency array react-hooks/exhaustive-deps. Obviously if I were to add those to the dependency list, I wouldn’t be able to change anything in the inputs, so how come I get this warning?

Summary

This is a question in two parts: first one about an idiomatic solution in React, as well as feedback to the React team: this scenario is simple and common, but it’s difficult to know how to implement correctly and safely in a consistent way.

Lifting state up and making the problematic component stateless is good advice that solves the problem, but every time it seems like a “temporary” solution. It also leads to painful refactoring every time something has to be moved around the component tree, so relying on it in the long run is extremely brittle.

The second part of the question is whether the solution with useEffect is viable at all, and in this case - why do I get the linter warning? Clearly I want to run it only when a certain prop changes. Is there an edge-case where this would result in an unexpected bug?

[sandeepmishra95]

@Yakimych At my current project we also faced a similar issue.

Lets say we have a list of Posts and when the user selects one of them, we show an edit Post component. From what I understand, this flow is quite similar to yours. What we did to solve the same issue that you were facing was to make the editor a stateful Component and used what we call the seeding pattern. In seeding pattern, the initial state of component depends upon the initial props and then the change in props are neglected entirely. We also mount the component with a key, so when another item is to be loaded in editor, we change the props and the key and a fresh Component Instance is mounted.

This also helps us to avoid repeating ComponentDidMount logic in case we had to use ComponentWillRecieveProps/getDerivedStateFromProps to detect props change.

Would like to know from you and others if this is an effective solution?

[Yakimych]

@quietwolf95 Yes, sounds like the exact same scenario.

Right, almost forgot about this - you can indeed use a random key to force the issue. My understanding was that it was more of a "last resort" approach in case nothing else works, but maybe I a wrong. I suppose one disadvantage with this approach is that it will pretty much always recreate the component (and the entire component subtree down from it) when the parent re-renders, and there would be no way to solve potential performance issues?

[nmain]

I don't quite like the way this came out; code is too verbose and awkward. But I feel it could be cleaned up / reorganized / encapsulated in some way.

https://codesandbox.io/s/jvr3rwj773

The child component can track the id of the thing it's editing, so you need some sort of stable way to refer to items, but everything else is kosher and there's no useEffect required.

To be honest, if I had to do this I'd probably use a key={item.id} from the parent. They say not to tie too much of your app logic to mounting/unmounting, but it's so easy!

[Yakimych]

I think I understand how it works - you're basically taking the fresh id from props when typing. This looks vaguely similar to some advanced patterns I've seen @kentcdodds mention (e.g. controlled props).

This is clever, but I agree with you, somewhat verbose and rather hard to understand. There are two things that are a bit alarming:

• we rely on the fact that changeId is called in every event handler for every field. If we forget one, we're introducing a source of bugs that is difficult to track down.
• each onChange event handler has to set all the other ones to the actualXYZ values. With 10 fields, the code will grow to 100 lines :) Also, as in the previous point, missing one of 100 will lead to a "hard-to-track-down" bug. But maybe you are right, there might be a way to clean up or reorganize?

[malerba118]

Here's what I would do. In general I agree with solution 2, but I'd tweak it.

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

const StatefulEditorSafe = props => {
  const [editableItem, setEditableItem] = useState(props.item)

  useEffect(() => {
    setEditableItem(props.item)
  }, [props.item.id]);

  const setItemField = (field, val) => {
    setEditableItem(prevState => ({
      ...prevState,
      [field]: val
    }))
  }

  return (
    <div className="editor">
      <input 
        type="text" 
        value={editableItem.name} 
        onChange={e => setItemField('name', e.target.value)} 
      />
      <textarea
        value={editableItem.description}
        onChange={e => setItemField('description', e.target.value)}
      />
      <div className="button-container">
        <button
          onClick={() =>
            props.onConfirm(editableItem)
          }
        >
          Ok
        </button>
        <button onClick={props.onCancel}>Cancel</button>
      </div>
    </div>
  );
};

export default StatefulEditorSafe;

My thoughts:

• there's no need to check props.item.description !== description or props.item.name !== name because the state setter from useState checks for equality and bails out if the value has not changed.
• I like to group state together whenever I can because it tends to be more concise and you can be sure that all pieces of state are updated in unison.
• I like to provide field names as an argument when updating form data. Either (field, value) => {...} or field => e => {...}. I don't really have a preference between the two.
• I'd check against the item id in useEffect instead of just the item object.

Also, if this is functionality you're going to want in various components, i'd extract it out into a custom hook.

const useStateWithDynamicDefault = (defaultVal) => {
  let [state, setState] = useState(defaultVal)

  useEffect(() => {
    setState(defaultVal)
  }, [defaultVal])

  return [state, setState]
}

[Yakimych]

@malerba118 This looks clean and straightforward, thanks! I do get a linter warning though:

React Hook useEffect has a missing dependency: 'props.item'. Either include it or remove the dependency array.

So I suppose I can just change [props.item.id] to [props.item] in the useEffect deps array? Any gotchas here?

[malerba118]

Yes, you can change [props.item.id] to [props.item]. I'd recommend having a look at Dan's A Complete Guide to useEffect to understand the intricacies of useEffect dependencies. One of the best articles i've seen in a while.

[threepointone]

I would recommend using a key to reset state in a master-detail view like this. It shouldn't be as expensive as you think, and you can consider optimizing it later if it does give you perf issues. Also, consider that it would React doing the work that you're trying to do manually here.

This is more of a usage question, and not a bug or feature request, so I'm going to close this issue. Feel free to carry on the discussion, or use a community resource for more support https://reactjs.org/community/support.html

[gaearon]

I missed this issue. The way you implement a componentWillReceiveProps-like pattern is described here:

https://reactjs.org/docs/hooks-faq.html#how-do-i-implement-getderivedstatefromprops

You don't need useEffect for it at all. Just set state during render.

[thibaultboursier]

@gaearon Setting the state in this way, apart from an effect, does not pose any particular problem with the concurrent mode?

[Yakimych]

@gaearon - Thanks, this is indeed much simpler. Speaking of which, I've been simplifying some scenarios by setting state during render in other cases, and ran into the same question as in this issue: apollographql/react-apollo#2287
Even more specifically, in my case it would be as setting state between lines 41 and 42 in this file: https://github.com/skanberg/graphql-apollo-demo/blob/2a30ed6d4308627f930f4861bb4152d8d6c69d51/src/App.js#L41.

Are there any potential gotchas when settings state inside an Apollo updateQuery function like this?

@thibaultboursier - Dan can correct me if I am wrong, but since React can track the state, and we're not doing anything particularly weird in this situation, this should be handled correctly with concurrent mode.

[mikeaustin]

While resetting a component using a new key is usually “good enough”, that approach can’t be used for example when dragging an SVG around and updating text fields. I do like the idea of uncontrolled on focus, and controlled on unfocused that I saw in another post.

[dimosmera]

useEffect seems simpler than keeping track of the old value, comparing, and then setting state during render.
@gaearon are there any dangers we are missing if we use useEffect for this?