Removal of onSuccess causes a lot of useEffects

[oren-legit]

I am in the midst of upgrading to version 5.
I did find a lot of use cases in our code that used onSuccess and in my opinion were clear flows that don't match anything described in the article as the reasons for using the callback.

And in each one of these cases, I find myself having to use useEffect, which I generally try to avoid because of it's readability and maintainability side-effect nature.

For example, I have a Select component that accepts selectedOptionsIds and setSelectedOptionIds, and I want to populate the selectedOptionIds with data from the server as the initial data for the select.
In that case, the state must remain a React state (because the user can select/deselect items), but I do need to run some code when the query returns successfully.

I can't find any other solution than useEffect here...

Here's another example from our codebase:

const [selectedOwner, setSelectedOwner] = useState<
    DetailedCollaboratorInformationDtoCollaborator | undefined
  >(undefined);

const {
    isLoading: ownerIsLoading,
    isError: ownerIsError,
    data: ownerData
  } = useQuery({
    queryKey: [repositoriesQueryKey, 'owner', repositoryId],
    queryFn: () =>
      repositoryApi.repositoryGetRepositoryOwner({
        id: repositoryId
      }),
    onSuccess: resp => {
      setSelectedOwner(
        resp.data.ownerOverride ? resp?.data.ownerOverride.id : resp?.data.owner.id
      );
    }
  });

return <ul>
  {ownerData.map(owner => <li key={owner.id}>
    <Button onClick={() => setSelectedOwner(owner.id)}>Set {owner.name}</Button>
  </li>
  )}
</ul>

Should I just use useEffect in all of these cases? It smells of bad code... Specifically because in all these cases I have to set state from within the useEffect.

[TkDodo]

the general idea here is to derive state:

const [selectedOwner, setSelectedOwner] = useState()

const { data: ownerData } = useQuery

const data = selectedOwner ?? (ownerData?.data.ownerOverride ? ownerData?.data.ownerOverride.id : ownerData.data.owner.id)

the selectedOwner is either the data that was selected by the user, or you fall-back to the server data.

If that doesn't work for you, you can also use the getDerivedStateFromProps pattern described in the react docs, where you call setSeletedOwner during rendering rather than in an effect:

• https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
• https://react.dev/reference/react/useState#storing-information-from-previous-renders

And additionally, your case is a good example of why we removed the callback. I described that in detail in my blog post, but if you add a staleTime to your query and change the repositoryId, you'll switch between queries but onSuccess would not run at all. So your component state got out-of-sync pretty easily.

[oren-legit]

Thank you Dominik!

What about more complex cases like the first one described?
Would you also use the same principles?

Like this?:

const [selectedOptionIds, setSelectedOptionIds] = useState([]);

const { data: defaultSelectedIds } = useQuery

if (selectedOptionIds.length === 0) {
  setSelectedOptionIds(data)
 }

The obvious problem here is that the user may choose to deselect all options, and will get the default options from the server - which is not what we want.
I want to set an initial value from the server, and then completely pass the control to the user, to select or deselect whatever.

Still trying to wrap my head around this new form of thinking

[TkDodo]

I have a blogpost on this topic: https://tkdodo.eu/blog/react-query-and-forms

I would prefer the "derived state" solution still:

const [userSelection, setSelectedOptionIds] = useState(undefined)

const { data: defaultSelectedIds } = useQuery

const selection = userSelection ?? defaultSelectedIds ?? []

It's the most simple code. Here, we start the userSelection with undefined, because the user hasn't selected anything. Then, we fall back to server state when that's undefined. That means when the query is loaded, the user will see the defaultSelectedIds as selected. As soon as they change something, userSelection will be different than undefined, so they will always see the user selection. If they de-select everything, it is probably empty array, which is still not undefined.

The concept of "default selection as state" doesn't make much sense to me. The user hasn't actually selected anything, why do we want to "store" that as a selection. For example, how would you do "dirty" detection with your approach? You don't know if the selected ids in the state are there because they come from the server default selection, or because the user has actually selected them ... you loose that differentiation when moving server state into client state.

Still, if you want that, splitting up components is second best:

function Component() {

  const { data: defaultSelectedIds } = useQuery

  // check for loading and error, and then:

  <MySelect initialData={data} />

}

function MySelect({ initialData }) {
  const [selectedOptionIds, setSelectedOptionIds] = useState(initialData)

  ...
}

[joshuajaco]

This is by far the dumbest API change in any library I've ever witnessed

[TkDodo]

It’s not even the dumbest API change in this library 😂