Lazy fetching of data will cause infinite loop / runaway effect #186
Comments
|
Hi @owinter86! Thanks for your contribution! I have a question – why would you add |
|
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? |
|
While I agree that it will make people's lives easier and improve the DX to have these functions included in the 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? |
|
While I agree that a user should not just blindly add items in the dependency array, like 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. |
|
I would expect const [isValid, setIsValid] = useState(true);
<ExpensiveComponent refetch={refetch}> |
|
Yes I think @stereobooster's example illustrates this issue in a better way, my useEffect example probably missed the mark a bit. |
|
Let's do it. @owinter86 would you like to open a PR? Happy to support you if you need it. |
|
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. |
|
I also want to give some more thought about it it simply refetch new data as a side effect of
And currently I'm stuck at resolving it, any help are welcome |
|
@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 😃 |
|
You can just remove If nothing change in the params of the With this kind of usecase, I could also suggest to use 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]); |
|
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) |
|
The problem with this is that the |
|
I will try to give a try to make |
|
Just want to mention that the same issues happens with 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 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 |
|
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 😁) |
|
@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 |
|
This definitely bit me tonight. I, unfortunately, have to use GQL and Rest so Apollo 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. |
|
@johncblandii Thanks for the feedback, I really need to spend some time on this issue… Another approch is to play with the 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) |
|
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. |
|
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! |
|
@AsasInnab Since we are using the 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! |
Hopefully begins to address contiamo#186 Approach: * Remove (mutable) references to state. Instead, hook into the (immutable) `setState` function in the mutate function body where state references are necessary * Try very hard to be honest about what dependencies may change in the dependency array, but leave only immutable dependencies that depend only on props (almost totally successful)
Hopefully begins to address #186 Approach: * Remove (mutable) references to state. Instead, hook into the (immutable) `setState` function in the mutate function body where state references are necessary * Try very hard to be honest about what dependencies may change in the dependency array, but leave only immutable dependencies that depend only on props (almost totally successful)
Stabilizing the identity of the `fetchData` function presents some
challenges. The biggest is that `fetchData` needs to have access to
`state` and `setState`, but it needs to have the most up-to-date version
of them in order to work. There are two ways to have done this: the
first is to move the definition of `fetchData` into the function body of
`useGet` itself and make it a closure over the `state` and `setState`
variables - that is the approach taken here. Alternatively, we could
define `fetchData` as a function which takes `state` and `setState` as
parameters, but is closed over a particular combination of props and
context values - this option wasn't explored.
The end result is that the identity of the `refetch` function is stable
given a certain set of props and context, and it is stable over a deep
comparison thereof - so both of the following work fine:
```typescript
const queryParams = { page: 1 };
function MyComponent = () => {
const { refetch } = useGet({ queryParams, lazy: true })`
...
};
```
```typescript
function MyComponent = () => {
const { refetch } = useGet({ { page: 1 }, lazy: true })`
...
};
```
This should resolve contiamo#186 fully (I hope!)
Stabilizing the identity of the `fetchData` function presents some
challenges. The biggest is that `fetchData` needs to have access to
`state` and `setState`, but it needs to have the most up-to-date version
of them in order to work. There are two ways to have done this: the
first is to move the definition of `fetchData` into the function body of
`useGet` itself and make it a closure over the `state` and `setState`
variables - that is the approach taken here. Alternatively, we could
define `fetchData` as a function which takes `state` and `setState` as
parameters, but is closed over a particular combination of props and
context values - this option wasn't explored.
The end result is that the identity of the `refetch` function is stable
given a certain set of props and context, and it is stable over a deep
comparison thereof - so both of the following work fine:
```typescript
const queryParams = { page: 1 };
function MyComponent = () => {
const { refetch } = useGet({ queryParams, lazy: true })`
...
};
```
```typescript
function MyComponent = () => {
const { refetch } = useGet({ { page: 1 }, lazy: true })`
...
};
```
This should resolve contiamo#186 fully (I hope!)
Stabilizing the identity of the `fetchData` function presents some
challenges. The biggest is that `fetchData` needs to have access to
`state` and `setState`, but it needs to have the most up-to-date version
of them in order to work. There are two ways to have done this: the
first is to move the definition of `fetchData` into the function body of
`useGet` itself and make it a closure over the `state` and `setState`
variables - that is the approach taken here. Alternatively, we could
define `fetchData` as a function which takes `state` and `setState` as
parameters, but is closed over a particular combination of props and
context values - this option wasn't explored.
The end result is that the identity of the `refetch` function is stable
given a certain set of props and context, and it is stable over a deep
comparison thereof - so both of the following work fine:
```typescript
const queryParams = { page: 1 };
function MyComponent = () => {
const { refetch } = useGet({ queryParams, lazy: true })`
...
};
```
```typescript
function MyComponent = () => {
const { refetch } = useGet({ { page: 1 }, lazy: true })`
...
};
```
This should resolve #186 fully (I hope!)
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:
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.
The text was updated successfully, but these errors were encountered: