Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to disable the rule react-hooks/exhaustive-deps? #6880

Closed
raRaRa opened this issue Apr 23, 2019 · 68 comments

Comments

@raRaRa
Copy link

commented Apr 23, 2019

I'm trying to disable the rule react-hooks/exhaustive-deps. I've tried adding "react-hooks/exhaustive-deps": "off", to .eslintrc without any luck.

How do I disable this rule? Thanks!

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

You can put // eslint-disable-next-line react-hooks/exhaustive-deps before the violating line if there is a good reason. Usually disabling it is a mistake and will significantly bite you later. (Most cases where people think it's safe to disable are not safe and lead to stale closures.)

Some references for how to write code with correctly declared dependencies:

If you post a complete example I can try to look at how to fix your case.

@raRaRa

This comment has been minimized.

Copy link
Author

commented Apr 23, 2019

Thanks for the quick response @gaearon. I've been through those links you mentioned.

I've got about 60+ of these warnings throughout my app. Most of the cases involve a dependency that doesn't update the effect, e.g. when calling redux actions, props.someAction(). Can you explain why I need the prop functions in the dep list, is it to make sure I get the latest/correct ref to the function, as it might change?

I have a lot of CSS animation logic related to when a slide enters or leaves a slideshow, where I only need to know if one or two dependencies changed (isEntering, isLeaving). Is there any good reason why I need to list deps that I know aren't needed? If I list the rest of the deps then those might mess up the animation logic unless I add more checks in the effect which makes it a lot more complex than it needs to be. (at least in comparison with class components)

I could list a few examples if you would like, but like I explained above it's mostly prop functions causing these issues for me.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Can you explain why I need the prop functions in the dep list, is it to make sure I get the latest/correct ref to the function, as it might change?

Yes. Functions capture state and props inside of them. It just so happens that React Redux currently uses classes (or does it even anymore?) with methods that have stable identity. (And in that case adding them to the array doesn't hurt either.) But in general, you shouldn't expect that calling an arbitrary function from an effect is safe without declaring that function as a dependencies. Here's an example where "skipping" a function dependency produces a bug: facebook/react#14972 (comment). (This particular example is about memo but useEffect would have the same issue.)

I have a lot of CSS animation logic related to when a slide enters or leaves a slideshow, where I only need to know if one or two dependencies changed (isEntering, isLeaving). Is there any good reason why I need to list deps that I know aren't needed?

I'd be happy to look at a concrete case if you can provide one. Maybe the lint rule itself has some false positives that we can fix?

@WesCossick

This comment has been minimized.

Copy link

commented Apr 23, 2019

For us, one of the primary places we're receiving this warning is for useEffect hooks that we only want to run when the component mounts, and never again for that component.

As a concrete example, the app we're working on can display a small notification in the top right corner that automatically disappears after 15 seconds. Here's a super trimmed down version of what that might look like:

// Imports
import React, { useEffect } from 'react';


// Export component
export default (props) => {
    // Handle hiding this notification
    const hideSelf = () => {
        // In our case, this simply dispatches a Redux action
    };
    
    
    // Automatically hide the notification
    useEffect(() => {
        setTimeout(() => {
            hideSelf();
        }, 15000);
    }, []);
    
    
    // Return JSX
    return (
        <div className='notification'>
            Just an example <button onClick={hideSelf}>Close button</button>
        </div>
    );
};

This complains because hideSelf is not a dependency listed in the useEffect hook. But that's intentional; we only want that hook to run when this component mounts.

Is there a better way to design this? Or would this be a false positive?

@WesCossick

This comment has been minimized.

Copy link

commented Apr 23, 2019

I suppose we could wrap hideSelf in useCallback and then place the memoized function in the dependencies list, but that seems a bit misleading to someone reading the code in the future. The empty brackets very clearly indicate that the effect only runs when the component mounts. But placing a dependency in the array would imply otherwise, even if the effect never actually re-runs. Plus, it would actually be undesirable for the effect to run a second time, so we'd want the code to not allow that possibility in the first place.

@raRaRa

This comment has been minimized.

Copy link
Author

commented Apr 23, 2019

The empty brackets very clearly indicate that the effect only runs when the component mounts

This is one of the reasons I don't want to add "unnecessary" things to my deps, as it makes the code look more complex and confusing. I totally agree that empty brackets tell you that it's CDM. There are many cases in my app where I just want to fetch once when the component mounts, but now I need to choose whether I want to add bunch of // eslint-disable-next-line react-hooks/exhaustive-deps lines or add the props as deps, but then I need to make sure the effect doesn't run any code when one of the deps changes. @gaearon I believe this is the main problem with this rule or hook. Perhaps we just need more hooks or options?

@raRaRa

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

I understand that we need to re-think how we write our components using hooks, instead of trying to figure out how class lifecycle methods (CDM, CDU, etc.) correspond to hooks. But I must admit that this rule has made me feel less happy about hooks, mostly due to the complexity around simple things such as CDM. Like discussed above, I just want to use empty brackets in the deps knowing that the logic inside the effect will run once during mount. But now with the eslint rule I can't do that unless I start adding the // eslint-disable-line all over the place. I'm confident that I'm not the only one running into this problem, more people will probably realize this as they upgrade to CRA 3.0.

I've looked at many libraries using hooks and most of them violate this rule. When I use the useEffect hook I think about what dependencies I want to trigger the effect. But this rule forces me to add all the dependencies, making the code less readable/predictable, forcing me to add more logic inside the effect to make sure it doesn't run again (CDM, etc.). I'm sure some people disagree, but how would you write a simple CDM and make it obvious to programmers on your team what the effect is doing? In my case empty brackets would tell me that immediately. The "right" way is probably to re-think this w/o thinking about the class lifecycle methods, by utilizing useMemo so that the deps don't change, etc. but IMO that could make it harder to understand. I want readable code over many things and class components gave us that. I was getting pretty comfortable with hooks until now.

This just doesn't feel right. I'm now forced to add // eslint-disable-line to over 60 lines, because I want to understand my code.

My suggestion would be to add new hooks or an option that would disable this rule, before more people start running into this same problem. We are used to the class lifecycle methods, perhaps we just need more official hooks from the React team that correspond to them. E.g. useEffectOnce that doesn't need deps.

@SCasarotto

This comment has been minimized.

Copy link

commented Apr 24, 2019

@WesCossick Is there a reason you can't move the hideSelf inside the useEffect? If you can then it wont ask for it to be in the dependency list.

@raRaRa

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

@WesCossick Is there a reason you can't move the hideSelf inside the useEffect? If you can then it wont ask for it to be in the dependency list.

But then he would need to add the Redux action to the dependency list, since the hideSelf func dispatches a Redux action.

@SCasarotto

This comment has been minimized.

Copy link

commented Apr 24, 2019

Good point @raRaRa. So in this post and example @gaearon uses a useRef to store a callback and doesn't pass it into the dependency list.

https://overreacted.io/making-setinterval-declarative-with-react-hooks/
https://codesandbox.io/s/3499qqr565

So could @WesCossick do the same?

@raRaRa

This comment has been minimized.

Copy link
Author

commented Apr 24, 2019

Yeah he could do the same by storing the Redux action in a useRef.

For me the main problem here is the complexity around achieving simple lifecycle methods such as componentDidMount without having to do all these tricks. I can't imagine going through this as a beginner in React. Another potential problem is code readability.

Imagine going through this code as a beginner:

Alright here's an useEffect, ah it has empty brackets so it's doing something like componentDidMount, hmm it's calling something within an useRef, ah the Redux action is stored in the useRef, etc.

I wouldn't want to be in his position.

@WesCossick

This comment has been minimized.

Copy link

commented Apr 24, 2019

@SCasarotto, in addition to what @raRaRa stated, it wouldn't be possible to move hideSelf into useEffect because it's also referenced by the onClick prop in the JSX. It would no longer be visible to that scope.

In regards to using useRef to store those functions, while that does appear to suppress the errors in this particular situation, it seems a bit... hacky. It's a misleading design wherein useRef is being used for something it's not intended for. According to the docs, useRef is "handy for keeping any mutable value around." The key word being mutable—which hideSelf is not. Nor is props.dispatch, from Redux.

Further, this and the previously mentioned useCallback method would both add a significant amount of boilerplate to nearly every component, especially ones that use Redux, thereby defeating one of the primary benefits of hooks.

I'd argue that the way it's designed presently, static code analysis for the react-hooks/exhaustive-deps rule simply isn't intelligent enough to make proper warnings (except for maybe the most basic uses of the useEffect hook). I'd argue that it takes human understanding of the nuances of the code to truly decide which dependencies should or should not be included.

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented Apr 24, 2019

I converted a large project to hooks and ran into the same issues as everyone else (200 class components). While initially I was skeptical about this rule, I ended up fixing all the warnings and it felt pretty good at the end. I think Dan's point that if you just have a useReducer with an empty dependency array, you are probably writting buggy code. In reality if you use redux and passing in some dispatch action for example, it felt unnecessary to list it as a dependency. While I agree with this sentiment, it does make you get into the habit of doing the right thing.

There were some issues, like I had a helper function that took in props, and if I just use the auto-fix feature it would pass in props as a dependency and cause an infinite mounting bug. So I ended up disabling auto-fixing for this rule in lint-staged. Also, refactoring these types of code felt pretty good but they can be very difficult to do if you have huge cDM and cDU functions.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

So far in our experience the issues caused by missing dependencies have been significantly worse than losing the quick “[] means mount” visual shortcut.

One thing I’ve noticed is that it’s often Redux users who complain about this. I think it’s likely because React Redux encourages the “action creator” pattern where you get a bunch of methods and then call them individually. Of course, declaring every method and thinking about them can get confusing if you have dozens.

You wouldn’t have this problem if you weren’t using the “action creator” pattern. For example, with useReducer our recommendation is to put dispatch itself on its own context. Then any effect that needs to dispatch something just needs [dispatch] as dependencies which is easy to read and remember it never changes in practice. This is not currently idiomatic in React Redux — but Hooks were primarily designed to be idiomatic in React itself. React Redux adjusted to past React idioms, and I ancitipate it can adjust to this too.

Finally, please remember nothing prevents you from creating your own useMount Hook that does what you want, and then calling it. The lint rule won’t check it. In that case you’re explicitly choosing to forgo checks because you consider the verbosity of how your chosen idioms (like React Redux) combines with Hooks worse than the stale props or state.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Regarding the hideSelf example above. It’s a bit tricky to discuss because there is no wider context. (Why do you keep UI state in Redux? Why does a component “hide itself” but stays mounted? Why isn’t this managed by its parent which shows the current list of notifications and cleans it up?)

But in this specific example the idiomatic solution is to put hideSelf inside the effect. If it dispatches a Redux action then put this action as a dependency. As you said, you expect Redux actions to “never change” — therefore there should be nothing bad about declaring them as deps. And if in the future this prop starts changing, your code will handle that.

I’d be happy to look into this in more detail if there was a sample project with a complete example.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

By the way. I know some people have very strong negative reaction to suppressing lint rules. Maybe it’s cultural and depends on a project.

We also had ~60 suppressions when we added this lint rule to FB codebase. Arguably we’re the biggest user of Hooks yet as we started earliest and use them very actively. Out of those 60 suppressions, about half were either actual bugs or bugs waiting to happen. For most of the rest, there was usually a way to restructure the code in a more idiomatic way as to not trigger the warning and make the code itself more resilient. I think by now we have only about ~30 suppressions left and people rarely introduce new ones.

We think the tradeoff is worth it and we don’t feel bad about adding suppressions to:

  1. Existing code you don’t have time to analyse or fix
  2. Places where you really think you know what you’re doing

I know some teams might have zero tolerance for suppressions which IMO is counterproductive but I could see how in such a climate this strategy is harder to make work. However again, you always have an escape hatch of making your own Hook with explicitly different semantics.

@audiolion

This comment has been minimized.

Copy link

commented Apr 25, 2019

Could there be some semantic thing to the rule where specifying a literal like ['onlyOnMount'] signifies you want to opt-in to didMount behavior and doesn't throw a warning?

sandbox:
https://codesandbox.io/s/4xowvm7nr9

example:

import React from "react";
import ReactDOM from "react-dom";

function App() {
  const [, setCount] = React.useState(0);
  React.useEffect(() => {
    setInterval(() => setCount(c => c + 1), 3000);
  }, [setCount]);
  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <h2>Start editing to see some magic happen!</h2>
      <UseEffectDidMount />
    </div>
  );
}

function UseEffectDidMount() {
  React.useEffect(() => {
    window.alert("I only happen once (didMount)");
  }, ["onlyOnMount"]);
  return null;
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);
@WesCossick

This comment has been minimized.

Copy link

commented Apr 25, 2019

After going through one of our apps and restructuring everything to avoid errors, I do agree that it's appropriate to work around the warnings thrown for using Redux functionality in a Hook. For us, that meant destructuring the dispatch function (const { dispatch } = props;) and passing dispatch into the dependencies of the necessary useEffect Hooks. While this does end up producing redundant boilerplate code, it seems appropriate for Redux to adapt to React, rather than the other way around.

Even still, after the restructuring we did have to suppress the react-hooks/exhaustive-deps quite a few times, though, by placing // eslint-disable-line react-hooks/exhaustive-deps on offending lines. And maybe we should rethink the prevailing view that ESLint rule suppressions as 100% evil. You have a point @gaearon that they can be productive in select cases.

I did notice, though, that almost exclusively we were suppressing the rule for useEffect Hooks that ran on mount only. There's a variety of reasons why we need effects that run on mount only in our own apps, and the example I provided above is just one of those, but I think it's pretty safe to say that this is a common need for many apps.

Sure, we could roll our own solution and create a reusable useMount Hook, but I wonder if, since the need for this Hook is so (presumably) widespread, it'd be better if everyone wasn't reinventing the wheel each time and React provided this type of Hook built-in? Obviously that's not a Create React App concern, but the potential need is more apparent since the upgrade to CRA 3.0.

FWIW, we also make heavy use of a modified version of useEffect that runs on updates only, rather than updates and mounts. This would actually be a really great candidate for a built-in Hook, since by creating our own Hook we're missing out on the linting. In this case, the linting would actually be much appreciated. At present, we can't have linting without forgoing a custom Hook and writing heavily redundant code.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I'm happy to consider individual cases, like I did in facebook/react#14920. (I asked for feedback at that point — it's unfortunate that you didn't see that thread and didn't offer your examples earlier.)

Please let's keep the discussion grounded in concrete CodeSandbox cases because we've already learned discussing this in abstract doesn't work. I'd also prefer that these examples show real use cases and not abstract ones like #6880 (comment) does. You can use facebook/react#14920 (comment) as examples of how to explain your use case in a way that is descriptive enough to offer concrete advise.

As I mentioned earlier, the case of a component "hiding itself" is dubious, and it would make more sense to me if a parent "notifications list" component managed and cleaned up "old" notifications in state instead. I'm happy to look at more concrete cases which need to be in CodeSandbox form.

@WesCossick

This comment has been minimized.

Copy link

commented Apr 25, 2019

Hey @gaearon, we weren't using React at our company back on Feb 21 when you posted facebook/react#14920, so we couldn't have offered any feedback at that time.

I'd be happy to offer concrete CodeSandbox cases based on real use cases. But before I do so, I'd like to make sure we're on the same page.

This discussion has come a long way from its original question. In my most recent comment, I pointed out a common need in our apps is to have useEffect Hooks that either run on mount only or update only. For the first, we use useEffect and often suppress the ESLint rule. For the second, to avoid redundant code, we created a custom Hook.

Suppressing the ESLint rule is not necessarily bad, but seems undesirable. Creating a custom Hook sidesteps the ESLint rule entirely, which is definitely not desirable. I believe we agree on these two things, but correct me if I'm wrong.

With all this in mind, and assuming many apps would need a way for useEffect to run on either mount only or update only, I proposed that built-in support for these behaviors might be the best solution. If you agree, we should move this discussion to the React repository.

However, your latest comment seems to imply that it's almost always possible to refactor a use case so that it doesn't require mount only or update only behavior. And therefore it wouldn't make sense to even consider built-in support for those behaviors.

If that's a correct assessment of your comment, I'd be happy to provide CodeSandbox cases that demonstrate where we felt we needed those behaviors. At that point, I assume we'll decide whether those are indeed reasonable use cases or if the designs need to be refactored.

If that's not what your comment implies, and you don't want to consider built-in support for those behaviors, please explain what you would like the CodeSandbox cases to show.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

In my experience many cases people in which people want to get around the rule have more idiomatic fixes that don’t break it. But people don’t often see these solutions immediately because it takes a bit of a mental shift. Another thing I’ve noticed is that many proposed scenarios where somebody thinks they want “mount only” behavior have legit reasons to run on updates too. Such as when people forget props change over time, or don’t take parent state into account. My goal is to collect as many of those concrete scenarios as possible and to think through them all so that we have a set of recommendations I can later point people to. My intuition is that most of these recommendations will have idiomatic solutions instead of suppressing the rule. But maybe I’m wrong and I’d like to collect more examples where that doesn’t make sense too. If there’s a sufficient number or those maybe we can make the rule less aggressive, for example, or offer an easier way to partially suppress it. However, I haven’t seen enough cases that are justifiable yet. Hope that helps!

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

The notion that some things need to happen only the first time user sees something is true. However associating it with “mount” is often a mistake. Such as conceptually a page navigation is often considered a fresh “appearance” even if technically the same ProfilePage component gets reused with different props. So, for example, scroll logic that resets scroll “on mount” (as one would intuitively do) is incorrect. Similarly, there are opposite cases where something appears for the first time, but the render result relates to an update. Such as if you needed to measure layout and then do a cascading render. Technically it’s an update but conceptually your “only run once” logic should wait for it. So the distinction between “mount” and “update”, while very familiar, is often the wrong tool. But each individual case is different. This is why I’d like to see concrete examples where you want to break the rule, with explanations that are user-centric rather than mount/update-oriented.

@audiolion

This comment has been minimized.

Copy link

commented Apr 26, 2019

I went to go make a codesandbox to demonstrate mount-only behavior, but after writing it up, I realized that there was a different way I could write it that would make it more resilient to re-renders. I feel like the big idea that people are missing is that they can have an effect that runs on mount, and properly cleans up after itself, and re-runs when deps change.

I see what you mean, and some of us are (slowly) getting it. While we want to map lifecycles to a hook equivalent mentally to better understand them, it isn't a perfect translation and we really do need to change our mental model around this to making our effects resilient to re-renders which helps avoid the bugs that you have been showcasing when props change over time, but the component doesn't remount.

I feel like most cases where you think you only want to run an effect once, you could run it again as deps change if you pass an appropriate cleanup function to the effect.

@raRaRa

This comment has been minimized.

Copy link
Author

commented Apr 26, 2019

I think this all comes down to code readability & predictability. But I totally agree with @gaearon that it takes a bit of a mental shift to get this right.

I had an issue with useEffect where I was reading from the animation state and setting it again plus generating a style object from the updated animation, and If I had used animation as a dependency then it would have caused a recursion issue. So I fixed it by using the functional update form of setAnimation as explained here: https://reactjs.org/docs/hooks-faq.html#what-can-i-do-if-my-effect-dependencies-change-too-often

setAnimation((a) => {
    const updatedAnim = {
        ...a,
        depth: -355,
    }
    setStyleObj(generateStylesObj(updatedAnim))
    return updatedAnim
})

But is this safe? using the functional update form of setAnimation with another state setter inside.

And here's the code before the fix, which required the animation state to be dependency and caused recursion:

const updatedAnim = {
   ...animation,
   depth: -355,
}

setAnimation(updatedAnim)
setStyleObj(generateStyles(updatedAnim))

I'm basically generating a style object from the animation state. Another potential solution is to use useMemo that updates the style obj when the animation state changes:

const styleObj = React.useMemo(() => ({
    opacity: animation.opacity,
    WebkitTransform: `
        perspective(${animation.perspective}) 
        translate3d(${animation.offsetX}%, ${animation.offsetY}%, ${animation.depth}px)
        rotateZ(${animation.rotation}deg)`,
    transform: `
        perspective(${animation.perspective}) 
        translate3d(${animation.offsetX}%, ${animation.offsetY}%, ${animation.depth}px)
        rotateZ(${animation.rotation}deg)`,
}), [animation])

Would it be better/safer to use the useMemo trick rather than manually setting the style obj where animation changes?

To give context on how the styleObj is used:

<div style={styleObj} />

@WesCossick

This comment has been minimized.

Copy link

commented Apr 26, 2019

That makes sense, @gaearon. I'm relatively new to React, so I'm definitely open to rethinking how we approach design choices for our apps.

It seems most of the times we need the useEffect Hook to run on mount only or update only (vs. mount and update) is in the context of some type of state recovery upon navigation. Below is one example.

Recovering form state

As a user navigates around a single page app, the goal is to have form state recovered to what they left it as. This only applies to using the browser's forward and back buttons to navigate to points in history they've already visited.

Because it's a real world example, it's powered in-part by React Router and Formik.

Here's the CodeSandbox editor and here's just the rendered app. For browser history state to work properly, you cannot use the CodeSandbox editor's forward, back, or refresh buttons; you must use just the rendered app in your browser.

Recovery in action:

  1. Open the rendered app
  2. Fill out the first and/or last name fields
  3. Click the "Some other page" link
  4. Use your browser back button to return to the "Example form" page
  5. Text should be recovered

The form-persist.js file shows a useEffect Hook that runs when the form first appears and never again (i.e. "mount only," but we're avoiding those terms). This is responsible for recovering Formik's state from the browser's history. It breaks the react-hooks/exhaustive-deps rule intentionally.

That file also uses a custom useUpdateOnlyEffect Hook, which is a version of useEffect that only runs when its dependencies update. This is responsible for saving updates to the browser's history. It breaks the react-hooks/exhaustive-deps rule unintentionally, but we wouldn't know that because the linter doesn't work for custom Hooks.

If the useEffect Hook is given [props.formik] as its dependencies, it results in an endless loop (see console output). If the useUpdateOnlyEffect is turned into a regular useEffect Hook, the initial values are saved into the browser state before it's recovered, so the recovery ends up pulling the initial values and not what the user actually typed.

Possible alternative designs for the useUpdateOnlyEffect Hook

You might initially think to change the order of the two Hooks in the form-persist.jsfile. If the useUpdateOnlyEffect Hook comes second, it can be converted to a standard useEffect Hook and the linter will catch the unintentional rule break. However, this is fragile code. Correct me if I'm wrong, but as far as I'm aware, React does not guarantee that the order in which it executes Hooks won't change from one version of React to the next. What if in a future version React decides to execute Hooks in a different order? Or what if simply something about your own code causes the two Hooks to be executed out-of-order? The two Hooks would be at the mercy of the order they're executed in, and the order isn't something that's documented or guaranteed to be permanent.

Another alternative would be to debounce or otherwise delay the saving of Formik's state. But how long do you decide to delay it? 100ms? 500ms? What if the user's computer is really slow? Timing becomes a significant concern, as the recovery and saving could still occur out-of-order.

Finally, you could simply not use a custom Hook, and instead write the logic with refs everywhere you need a Hook that runs on updates only. But that creates an unnecessary amount of redundant boilerplate code. Hence, a custom Hook.

Possible alternative designs for the useEffect Hook

Unfortunately, I'm not aware of any design changes that could be made to the Hook that recovers the state such that it doesn't intentionally break the ESLint rule.


I'd love to hear your thoughts, @gaearon, on this concrete example. Is there a way we could approach this differently?

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@raRaRa Do you mind posting a full example for animation case that I could run? Usually when you start reaching for the functional setState, the code can be further simplified with useReducer.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@WesCossick

Thanks for a concrete example. I’m on my phone so can’t test it in depth. But I’m curious — is there any particular reason you want to fill in the initial state from an effect, as opposed to actually using the initial state for this? (In case of Formik, I imagine it has some prop to set initial values.) I think that would remove your need for a “first render only” effect to fill the state.

@gaearon

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Curiously your desire to break the rule here does uncover the problem in an app. I think that if you open it in two tabs, diverging edits from those would overwrite one another. So one of them wouldn’t get restored. While not likely, it can be a problem. To solve it you could store some kind of revision ID in the storage too. In that case you would want to run the effect on update too. Instead of always replacing the state, it would offer to either resolve conflict (if unexpected revision is in the store), or ignore the update (if revision belongs to this tab). That could let you keep tabs in sync.

https://overreacted.io/writing-resilient-components/#principle-3-no-component-is-a-singleton is a relevant principle.

Of course it might be a higher bar than most apps care about. But I’m just pointing out that even here, the lint rule points out a deeper flaw in the design and forces you to confront it. (Or hack around it.)

@WesCossick

This comment has been minimized.

Copy link

commented May 1, 2019

Hey @gaearon, the discussion has carried on quite a bit, but I wanted to see if you had any further thoughts on the state recovery example I provided? Is it possible to refactor that example?

@DominicTobias

This comment has been minimized.

Copy link

commented May 14, 2019

Regardless it would be good if we could turn off this rule in eslint for us people who want to live on the edge. It has good intentions but I don't want to unnecessarily memoize all the functions I use in an effect, i.e.

const authed = useStore(state => state.user.authed);
const setAuthModalOpen = useActions(actions => actions.app.setAuthModalOpen);

useEffect(() => {
  setAuthModalOpen(!authed);
}, [authed]);

I regularly call methods created in the render function, would you guys recommend memoizing them all? I think this would increase code complexity in this case? Or else it would be handy to choose to consciously ignore them (but not everything):

useEffect(() => {
  setAuthModalOpen(!authed);
}, [authed]); // ignore: setAuthModalOpen, fn2, ...

So far I've found I'm looking for variable changes, not functions.

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

The lint rule is able to catch most subtle bugs so overall I think it's a net win.

@bovas85

This comment has been minimized.

Copy link

commented May 14, 2019

I'm using firebase and the db dependancy is failing eslint, but I can't use that as dependancy because it updates every .2s (it's a realtime database).

Any ideas?

useEffect(() => {
    const updateStore = async () => {
      const goalsRef = db.collection(user.uid).doc('goals')
      const res = await goalsRef.get()
      const data = await res.data()
      const goalsArray = data && await data.goals

      if (goalsArray) {
        setGoals(goalsArray)
        return
      }

      await db
        .collection(user.uid)
        .doc('goals')
        .set({
          goals: [newGoal]
        })
      setGoals(goalsArray)
    }
    updateStore()
  }, [user]) // here [db] is missing, but adding it causes an infinite loop of the effect as db keeps updating
@SCasarotto

This comment has been minimized.

Copy link

commented May 14, 2019

I use firebase a lot. I just never store a reference to db I just always firebase.firestore()

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

Move declaration of db inside useEffect may help. You haven't shown how you defined db.

@bovas85

This comment has been minimized.

Copy link

commented May 14, 2019

@SCasarotto can you show an example of how you would query and update it with hooks? Moving the code above outside the component also works but I need some props to be sent to the method

@SCasarotto

This comment has been minimized.

Copy link

commented May 14, 2019

From one of my production apps I recently built with hooks:

import { useState, useEffect } from "react";
import firebase from "firebase/app";
import "firebase/firestore";

import { collectionToDataArray } from "../../../helpers";

export const useOrganizations = () => {
  const [organizationArray, setOrganizationArray] = useState([]);

  useEffect(() => {
    const usersWatcher = firebase
      .firestore()
      .collection("Organization")
      .onSnapshot(
        snapshot => {
          setOrganizationArray(collectionToDataArray(snapshot));
        },
        e => {
          console.error(e);
        }
      );
    return () => {
      usersWatcher();
    };
  }, []);

  return organizationArray;
};

As for "query and update" see something like:

useEffect(() => {
  const userWatcher = firebase
    .firestore()
    .doc(`Coordinator/${uid}`)
    .onSnapshot(
      snapshot => {
        dispatch({ type: "fetch", payload: docToDataObject(snapshot) });
      },
      e => {
        console.error(e);
      }
    );
  return () => {
    userWatcher();
  };
}, [uid]);

@bovas85

This comment has been minimized.

Copy link

commented May 15, 2019

Interesting @SCasarotto, why do you return the disposer to run the watcher?
Suppose I have an input that I change in client, how would you then update the firestore?

@gryn

This comment has been minimized.

Copy link

commented May 20, 2019

  useEffect(
    () => {
      if (previousLocation === 'foo-child') {
        window.scrollTo(0, scrollPosition);
      }
    },
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [previousLocation],
  );

(the goal here is to only scroll back to the saved location of this page if we are coming from a child page).

Both of these variables are in redux, but the effect should only be run when one of them has changed, not both of them. (effectively scrollPosition is constantly recorded, but meaningless, unless previousLocation has just changed).

Is this an appropriate use of the eslint-disable? Is this the correct usage of an effect?

@mcmillion

This comment has been minimized.

Copy link

commented May 23, 2019

I've read through this thread twice now and I still don't quite grasp a clean and terse way to use useEffect to simulate what I use componentDidMount for (kicking off Redux actions to fetch data, etc). It feels weird that the cleanest solution to this seems to be to go back to just using a class component.

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

For this issue, we aren't planning to [optionally] disable react-hooks/exhaustive-deps because its advantages outweighs any inconveniences. If you are migrating old code, you are better off fixing these warnings rather than trying to disable them. If you need help, please create a codesandbox rather than a small code snippet and we may be able to help. Thanks.

@bugzpodder bugzpodder closed this May 23, 2019

@gryn

This comment has been minimized.

Copy link

commented May 23, 2019

@bugzpodder Could you please comment on the use case I posted above, thank you.

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented May 23, 2019

@gryn please provide a minimal repro using codesandbox.io rather than a code snippet.

@samuelmataraso

This comment has been minimized.

Copy link

commented May 26, 2019

useEffect(() => {
getResults();
// React Hook useEffect has a missing dependency: 'getResults'. Either include it or remove the dependency array react-hooks/exhaustive-deps
}, [query]);

https://codesandbox.io/s/vdevr

@bovas85

This comment has been minimized.

Copy link

commented May 27, 2019

@samuelmataraso in that case you need to add that function as dependancy or declare it either outside of the component or inside the use effect and then call it

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

@samuelmataraso as @bovas85 said, you can do this:

  useEffect(() => {
    const getResults = async () => {
      const response = await axios.get(
        `http://hn.algolia.com/api/v1/search?query=${query}`
      );
      setResults(response.data.hits);
    };
    getResults();
  }, [query]);
@gryn

This comment has been minimized.

Copy link

commented May 28, 2019

@bugzpodder I'm not sure how complete you would want this example. I have left a large comment explaining the purpose of the effect: https://codesandbox.io/s/infallible-hellman-mdech

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

Hi @gryn, the codesandbox should be a working app demonstrating real app behavior. Without it, we can't really play around with it and can only guess in terms of what the actual fix to be. If the codesandbox is just a stub, then it defeats the purpose of making it.

@gryn

This comment has been minimized.

Copy link

commented May 30, 2019

@bugzpodder I have updated the example to be "fully functional" (I still have not opted to include redux, but have stored the state in the top level component as a substitute). I have added documentation to the entire file, including what you should do to exercise the critical path.

@bluepeter

This comment has been minimized.

Copy link

commented May 30, 2019

MobX is a pretty good example of where injecting dependencies isn't necessary, but we get the annoying linting warnings with no way to turn them off.

Important realization here is that you will never need to specify dependencies for useEffect because there are simply none. Or to be more precise it does depend on store variable, but that's the same case as for React.useRef which value you shouldn't specify in dependencies also.

@bluepeter

This comment has been minimized.

Copy link

commented May 30, 2019

@bugzpodder

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2019

@gryn thanks for producing a working example. here is my fork of your sandbox: https://codesandbox.io/s/elegant-austin-qmqob
Here are the changes I made:

  1. checking location !== previousLocation in ScrollToTop doesn't make sense to me. It should always be different? (maybe this is not the case in your app)
  2. scrollPosition should not be stored in redux and passed via props. The reason is that if it changes, the component re-renders because this prop changed.
  3. What I did was changing scrollPosition to scrollRef from useRef. You can check out Dan's post on why this makes sense: https://overreacted.io/making-setinterval-declarative-with-react-hooks/

@bluepeter I don't have experience into mobx or why their documentation recommends an empty dependency array. In my experience with redux, the dispatch function never changes but I put it in the dependency array anyway. Does it work if you put the store in the dependency array (fully realizing that the docs did not recommend this approach)

@bluepeter

This comment has been minimized.

Copy link

commented May 31, 2019

@bugzpodder passing the stores into the dependency does seem to work. It seems somewhat inelegant, though.

@FredyC can probably articulate better than I can why it's not recommended to pass in MobX store dependencies into useEffect hooks?

@FredyC

This comment has been minimized.

Copy link

commented May 31, 2019

MobX "store" has stable referential integrity, it will never change unless doing that on purpose. It does work to specify it in dependencies, but as @bluepeter says, it's highly inelegant as it just clutters the code without any benefit.

I still haven't got into using ESLint with CRA, TSLint is good enough for now and because of these false positives I am probably going to avoid it for a long time. Similarly, @jaredpalmer had issues where it was forcing him to useCallback for unnecessary reasons. So thanks, but no thanks :)

@gryn

This comment has been minimized.

Copy link

commented May 31, 2019

@bugzpodder Jack, thank you for your feedback. I agree that the code is not excellent the way it is! I am working within a large application and sometimes we don't have the luxury of fixing what we find. However, I can fix the issue with the way the scroll saving works (we will still need to store it in redux, since the component recording the position will unmount when the page changes, but I can pass a ref down to the wrapped component, instead of a prop).

My take away from this is that you can use references for "consumable" information, and props for "actable" information. That is, "actable" means it either directly effects rendering, or can directly cause action. "Consumable", on the otherhand, is useful only during an action, and is not the cause of one.

Perhaps there are better words for it, and perhaps I am still missing some subtlety. In anycase, you've helped me move forward and hopefully improved my understanding. Thank you.

@lock lock bot locked and limited conversation to collaborators Jun 5, 2019

@gaearon

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

Since my comment about action creators got "buried" in the collapsed thread, I'll post it again here: #6880 (comment). TLDR: Hooks don't mesh very well with the "action creator" pattern. The recommendation is to useDispatch() directly. If you insist on splitting "presentational" and "container" components, you can always split them in two layers yourself, and put the useDispatch calls in the outer ones.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
You can’t perform that action at this time.