schiehll / react-alert

alerts for React
MIT License
607 stars 99 forks source link

Running `alert.show` inside `useEffect` causes infinite loop #119

Closed amillward closed 5 years ago

amillward commented 5 years ago

I have a functional component that loads data inside a useEffect. I'm required to provide alert as a dep, however doing so causes an infinite loop.

CodeSandBox here: https://codesandbox.io/s/mystifying-rubin-dhbov

besLisbeth commented 5 years ago

Hi, @amillward. Can you please, explain, why did you pass an alert to useEffect?

What can I tell you - in your code useEffect is calling on every rerender, as it is written in the documentation. Because of the fact, that you calling an alert - it causes rerender, useEffect is calling - alert starts showing, useEffect is calling - infinite loop. To prevent looping in such a situation as yours, you can pass the second argument into the useEffect. Rerender will be caused only when the second argument will become truthy. You passing there an alert and showAlert - they will always be truthy, because alert is storing a link.

So, you need to pass to useEffect only [showAlert]. When it becomes true - an alert will be shown without any looping. I'm adding a link to the official documentation.

amillward commented 5 years ago

Omitting alert as a dependency shows the warning; React Hook useEffect has a missing dependency: 'alert'. Either include it or remove the dependency array. (react-hooks/exhaustive-deps). I'm sure I could remove the eslint warning here, but I'd rather have a solution that doesn't require me to ignore warnings. https://overreacted.io/a-complete-guide-to-useeffect/#dont-lie-to-react-about-dependencies

besLisbeth commented 5 years ago

I found a solution for you in one of the React issues. Please, have a look into this sandbox in issue https://github.com/facebook/react/issues/15204 However, my personal preference would be to disable this rule (maybe).

amillward commented 5 years ago

ah yes - I actually tried wrapping with useCallback before posting but I wrapped alert instead of alert.show. It works here; https://codesandbox.io/s/nostalgic-bardeen-jnudf

But would it be appropriate to wrap these functions within the react-alert module itself? It should work the same I believe. Otherwise anyone who wishes to show an alert as a result of an effect - I assume fairly common for things like errors loading the content on a page - has to create this arbitrary wrapper every time.

besLisbeth commented 5 years ago

It won't work the same, because your component is a functional component. On every re-render, the new component with the new link to the showAlert function will be created. useCallback returns memoized function, so I suppose that it stores the link somewhere inside the React and it is always the same.

From my point of view, I'm considering the useEffect hook as 'componentDidUpdate' method of React class component, where we are passing properties comparison for preventing an internal loop.

I assume, that useCallback prevents the creation of the new functions. But it seems really ridiculous to me to pass it into the useEffect argument if look at it from the perspective of 'componentDidUpdate' method.

Anyway, I think, that you should take a look at these React issues: facebook/react#14920, facebook/react#15204 where people discuss the same problem as you have.

Also, I'm also closing this issue, because it's not the drawback of this particular repo.