Lazy fetching of data will cause infinite loop / runaway effect

[owinter86]

Describe the bug With a lazy useGet, placing the refetch within an effect will cause an infinite loop of fetches due to the refetch function getting regenerated on each render.

To Reproduce Steps to reproduce the behavior:

  const { refetch } = useGet(`/some-endpoint`, { lazy: true });

  React.useEffect(() => {
    refetch();
  }, [refetch]);

Expected behavior I believe that refetch should be memoised and should not fail the dependency check in an effect. SO that you can safely refetch data in an effect with the refetch function as a dependency.

[TejasQ]

Hi @owinter86! Thanks for your contribution! I have a question – why would you add refetch as a dependency to useEffect if you're don't want to rerender when it changes? Happy to memoize it on the library side, but this seems like an interesting pattern...

[owinter86]

Hey @TejasQ , I totally agree that the above effect should not have refetch as a dependency, I simplified this example to show the issue I am having.

This is more of a developer experience improvement that an actual bug, as the hooks eslint rule will flag this if refetch is not included in the dependency array, this will be a bit problematic especially if you have an effect with many dependencies that you want to trigger a refetch, and the hooks eslint rule gives piece of mind that you have not missed any dependencies in the effect. So you would have to disable this rule and remove refetch from the auto generated array.

I also see that the the internal apis for setState, and a useReducer dispatch allow you to safely add these as dependencies without the problem of being recreated on re-render. Also some external libraries manage this as well, react-redux is useDispatch is a good example.

I just feel like that many users who rely on this eslint rule to either auto fill the dependency array or use it to flag missing dependencies will need to disable it any time they would like to have refetch in the effect.

I think that it will likely improve DX if the library can memoise this function so it can safely be used with the hooks eslint rule, and given there is no return value, and its just updating another reference, I dont think it needs to be recreated each update.

What do you think?

[TejasQ]

While I agree that it will make people's lives easier and improve the DX to have these functions included in the useEffect dependency "watch list", I think it's quite misleading to just populate it with things inside the effect, even if we don't explicitly want to watch them. It's kind of abusing the injection mechanism of useEffect and, in my opinion, an anti-pattern.

But we probably don't want people to have to think of that and allow them to focus on their tasks at hand instead of worrying about library topics.

I wonder what the rest of the team things? @stereobooster @fabien0102 any ideas?

[owinter86]

While I agree that a user should not just blindly add items in the dependency array, like refetch(), but given the rules of hooks eslint will ship with CRA and react-native init projects, and that many "Popular" libraries will ensure that if a user does use these auto dependency injection, functions like refetch will generally be memoized.

This could be a seen as an anti pattern for users to blindly do this, or an issue with how the eslint rule picks up dependency's, but I do think that without the memoization, this issue will likely be raised a lot in this package, rightly or wrongly.

[stereobooster]

I would expect refetch to be referentially transparent the same way as in:

const [isValid, setIsValid] = useState(true);

setIsValid is always the same reference. I would not consider React.useEffect(() => refetch(), [refetch]) as main use case, but referential transparency affects how react components re-render.

<ExpensiveComponent refetch={refetch}>

[owinter86]

Yes I think @stereobooster's example illustrates this issue in a better way, my useEffect example probably missed the mark a bit.

[TejasQ]

Let's do it. @owinter86 would you like to open a PR? Happy to support you if you need it.

[owinter86]

Sure @TejasQ would be more than happy to help out, I should be more available towards the end of the week and can dive in then.

[tonthanhhung]

I also want to give some more thought about it

  useEffect(() => {
    if (user && user.id) {
      fetchNewData()
    }
  }, [user]);

it simply refetch new data as a side effect of user state changed my project also apply lint rule as documented here https://reactjs.org/docs/hooks-reference.html#useeffect

We recommend using the exhaustive-deps rule as part of our eslint-plugin-react-hooks package. It warns when dependencies are specified incorrectly and suggests a fix. Then I got and error as below for the above code

React Hook useEffect has a missing dependency: 'fetchNewData'. Either include it or remove the dependency array  react-hooks/exhaustive-deps

And currently I'm stuck at resolving it, any help are welcome

[fabien0102]

@tonthanhhung I guess we can fix the issue, not really have the time now for this sadly, I can propose you a little hack instead ^^

const { refetch } = useGet(`/some-endpoint`, { lazy: true });

  const prevUserId = React.useRef(user.id)
  React.useEffect(() => {
    if (user && user.id && prevUserId.current !== user.id) {
      refetch();
      prevUserId.current = user.id;
    }
  }, [refetch, user]);

Something like this should do the job. Let me know if it's working 😃

[fabien0102]

You can just remove lazy: true and the useEffect associate in this case.

If nothing change in the params of the useGet you will not have any refetch, so if it's a frontend filter, should works out of the box.

With this kind of usecase, I could also suggest to use useMemo instead of an other state + effects

Something like this:

const { data, loading, cancel } = useGetUsers();
const [searchTerm, setSearchTerm] = useState<string>("");

const filteredUsers = useMemo(() => {
  if (!data) return [];
  const regex = searchTerm ? new RegExp(searchTerm.toUpperCase(), "g") : /.*/;
  return data.user.filter(u => (u.name || "").toUpperCase().match(regex))
}, [searchTerm, data]);

[hydrosquall]

I experienced the same issue today about the refetch not maintaining referential transparency, resolved it by memoizing just the refetch (modifying based on @fabien0102 's example above)

const { data, refetch: refetchRaw } = useGetUsers(queryParams: { name: props.name });
const refetch = useCallback(refetchRaw, [props.name]);

[zeorin]

The problem with this is that the refetch will now use the stale context and state, meaning that momentarily, whilst the network request is still pending, the state and context as it was when the first refetchRaw was wrapped in useCallback.

[fabien0102]

I will try to give a try to make refetch a bit more stable 😉

[WonderPanda]

Just want to mention that the same issues happens with mutate when dealing with hooks for POST endpoints. From my experience there's a general expectation that values returned from hooks should be safe to use inside of dep arrays for useEffect, useCallback etc. All of the built in React hook functions (useCallback, stateSetFunction from useState, useContext) as well core community libraries like React Router and Apollo make their hooks safe for this use case.

It's extremely common in most apps I've seen to want to do make an API call (whether a POST or to refetch some data) in response to some internal changes in the component which is exactly what useEffect is for but right now I keep having to disable linting which isn't ideal .

Overall though am blown away at by how great the library is thanks for all the hard work you've put in for the community

[fabien0102]

I want to say "fair enough", sadly we realized this a bit late, and it's really not trivial to change...

This issue is definitely on my priorities, but I need to think about an elegant solution, and find time to do it! (And we are of course open to PR if somebody is in fire 😁)

[WonderPanda]

@fabien0102 I'd be interested in helping out but ramping up on a large codebase can be time consuming. Any chance you can provide some insight/thoughts from your end as to why it wouldn't be a trivial change and what current design decisions might get in the way? Maybe we could start an RFC issue to discuss the best plan of attack?

I think this will be an important issue for this library as Hooks continue to see increased adoption and developer mind share. It definitely bit me a few times before I realized what was going on based on preconceived notions from all the other Hook libs I've used.

I'm happy to use the library in it's current state just disabling linting rules when it comes up but explaining it to more junior devs on the team and having to keep an eye out all the time in PRs isn't the most ideal. At the very least I think it deserves a mention in the Readme under Caveats or something like that so we can point people to some resources and potential work arounds. I'd be happy to help open a PR for some additions to the docs if you were okay with that as a starting point

[johncblandii]

This definitely bit me tonight. I, unfortunately, have to use GQL and Rest so Apollo use* hooks result a result you can use as a dependency to useEffect. Had I not randomly logged the results I wouldn't have caught repeated requests.

The solution I ended on was to use a callback as the dependency.

  const result = useGetUsers({ lazy: true });

  const loadData = useCallback(() => {
    result.refetch();
  }, []);

  useEffect(loadData, [loadData]);

It is extra, but it did stop the repeats.

[fabien0102]

@johncblandii Thanks for the feedback, I really need to spend some time on this issue… Another approch is to play with the lazy props itself:

const result = useGetUsers({ lazy: props.shouldFetch });

So far, we never have this issue in our product, and we have some non-trivial data flow (but we are also using a raw generated Promise by restful-react instead of hooks in some situation, really depends of the usecase)

[johncblandii]

I started to wrap the component and have the reason why it was lazy loading in a different component and then let the query run as expected, but I couldn't help fight to figure out a solution so we didn't have to create extra components across the app.

For now I think calling it out on the readme would be great. I went there to see if there was a note about effects.

[AsasInnab]

Any updates on this matter?

According to Dan Abramov it's anti pattern to leave out the useEffect dependencies, and it's adviced against unless there are valid reasons - I wouldn't consider this to be such a case. Thanks for a great library though! https://github.com/facebook/create-react-app/issues/6880#issuecomment-485912528

[fabien0102]

@AsasInnab Since we are using the lazy props for this usecase (as described https://github.com/contiamo/restful-react/issues/186#issuecomment-698154371) we are not really running in this issue.

I totally agree about the anti pattern, I did migrate this library to hooks when they release the feature and definitely missed this part in the design! This said, really sorry, but since this is not critical for our usage, I don't have the time to dig in this topic… But, I'm always happy to review some amazing PR!