[ESLint] Feedback for 'exhaustive-deps' lint rule

[gaearon]

Common Answers

💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡

We analyzed the comments on this post to provide some guidance: #14920 (comment).

💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡

---

What is this

This is a new ESLint rule that verifies the list of dependencies for Hooks like useEffect and similar, protecting against the stale closure pitfalls. For most cases it has an autofix. We'll add more documentation over the next weeks.

Installation

yarn add eslint-plugin-react-hooks@next
# or
npm install eslint-plugin-react-hooks@next

ESLint config:

{
  "plugins": ["react-hooks"],
  // ...
  "rules": {
    "react-hooks/rules-of-hooks": 'error',
    "react-hooks/exhaustive-deps": 'warn' // <--- THIS IS THE NEW RULE
  }
}

Simple test case to verify the rule works:

function Foo(props) {
  useEffect(() => {
    console.log(props.name);
  }, []); // <-- should error and offer autofix to [props.name]
}

The lint rule complains but my code is fine!

If this new react-hooks/exhaustive-deps lint rule fires for you but you think your code is correct, please post in this issue.

---

BEFORE YOU POST A COMMENT

Please include these three things:

1. A CodeSandbox demonstrating a minimal code example that still expresses your intent (not "foo bar" but actual UI pattern you're implementing).
2. An explanation of the steps a user does and what you expect to see on the screen.
3. An explanation of the intended API of your Hook/component.

But my case is simple, I don't want to include those things!

It might be simple to you — but it’s not at all simple to us. If your comment doesn't include either of them (e.g. no CodeSandbox link), we will hide your comment because it’s very hard to track the discussion otherwise. Thank you for respecting everyone’s time by including them.

The end goal of this thread is to find common scenarios and transform them into better docs and warnings. This can only happen when enough details are available. Drive-by comments with incomplete code snippets significantly drive down the quality of the discussion — to the point that it's not worth it.

[billyjanitsch]

This example has a reply: #14920 (comment)

CodeSandbox

This is an uncontrolled Checkbox component which takes a defaultIndeterminate prop to set the indeterminate status on initial render (which can only be done in JS using a ref because there's no indeterminate element attribute). This prop is intended to behave like defaultValue, where its value is used only on initial render.

The rule complains that defaultIndeterminate is missing from the dependency array, but adding it would cause the component to incorrectly overwrite the uncontrolled state if a new value is passed. The dependency array can't be removed entirely because it would cause the indeterminate status to be fully controlled by the prop.

I don't see any way of distinguishing between this and the type of case that the rule is meant to catch, but it would be great if the final rule documentation could include a suggested workaround. 🙂

[gaearon]

Re: #14920 (comment)

@billyjanitsch Would this work instead? https://codesandbox.io/s/jpx1pmy7ry

I added useState for indeterminate which gets initialized to defaultIndeterminate. The effect then accepts [indeterminate] as an argument. You currently don't change it — but if you did it later, I guess that would work too? So the code anticipates future possible use case a bit better.

[dan-lee]

So I got this following (edge?) case where I pass some html and use it with dangerouslySetInnerHtml to update my component (some editorial content).
I am not using the html prop but the ref where I can use ref.current.querySelectorAll to do some magic.
Now I need to add html to my dependencies in useEffect even though I am not explicitly using it. Is this a use case where I actually should disable the rule?

The idea is to intercept all the link clicks from the editorial content and track some analytics or do other relevant stuff.

This is a watered down example from a real component:
https://codesandbox.io/s/8njp0pm8v2

[lukyth]

const onSubmit = React.useCallback(
  () => {
    props.onSubmit(emails);
  },
  [emails, props]
);

I expect the lint to fix the deps into [emails, props.onSubmit], but right now it always fix the deps into [emails, props].

1. A CodeSandbox demonstrating a minimal code example that still expresses your intent (not "foo bar" but actual UI pattern you're implementing).

https://codesandbox.io/s/xpr69pllmz

2. An explanation of the steps a user does and what you expect to see on the screen.

A user will add an email and invite those email to the app. I intentionally remove the rest of the UI to just the button because they're irrelevant to my issue.

3. An explanation of the intended API of your Hook/component.

My component has a single prop, onSubmit: (emails: string[]) => void. It'll be called with the emails state when a user submit the form.

---

EDIT: Answered in #14920 (comment)

This is because technically props.foo() passes props itself as this to foo call. So foo might implicitly depend on props. We'll need a better message for this case though. The best practice is always destructuring.

[atomiks]

CodeSandbox

It doesn't consider that mount and update can be distinctly different when integrating when 3rd party libs. The update effect can't be included in the mount one (and removing the array altogether), because the instance shouldn't be destroyed on every render

[gaearon]

@joelmoss @sylvainbaronnet I appreciate your feedback but I hid your comments because they didn't include the information we asked for at the top of the issue. That makes this discussion unnecessarily difficult for everyone because there's missing context. I'd be happy to continue the conversation if you post again and include all the relevant information (see the top post). Thank you for understanding.

[btholt]

1. CodeSandbox
2. User can select a first name and it forces the last name to change. User can select a last name and it does not force the first name to change.
3. There's a custom hook which returns a piece of state, a select component that updates that state, and the state hook's update method that updates the state programmatically. As demonstrated, I don't always want to use the updater function so I left it to the be last item in returned array.

I believe the code as-is shouldn't error. It does right now on line 35, saying that setLastName should be included in the array. Any thoughts on what to do about that? Am I doing something unexpected?

[joelmoss]

I totally understand, and I would normally do all that for you, but in my specific case, none of the code is unique to me. It's simply a question about whether the use of a function that is defined outside of the hook (ie. a redux action creator) and used within a hook should require that function to be added as a hook dep.

Happy to create a codesandbox if you still need more info. thx

[btholt]

@joelmoss I think my repro covers your case too.

[gaearon]

Yes, a CodeSandbox would still help. Please imagine what it's like to context switch between people's code snippets all day. It's a huge mental toll. None of them look the same. When you need to remember how people use action creators in Redux or some other concept external to React it's even harder. The problem may sound obvious to you but it's not at all obvious to me what you meant.

[sylvainbaronnet]

@gaearon I understand it makes sense, actually it worked for me because why I wanted to achieve was :

If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument.

Thanks a lot for the clarification. (and sorry for the off topic)

[robbestad]

Here's the code version of what I'm implementing. It don't look like much, but the pattern is 100% identical with my real code.

https://codesandbox.io/s/2x4q9rzwmp?fontsize=14

• An explanation of the steps a user does and what you expect to see on the screen.

Everything works great in this example! Except the linter issue.

• An explanation of the intended API of your Hook/component.

I've got several files like this, where I'm executing a function in the effect, but I only want it to run whenever some condition is met - for instance, changing the series Id. I don't want to include the function in the array.

This is what I get from the linter when running it on the sandbox code:

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

My question is: is this how it should behave (either the linter rule, or the hook array rule)? Is there a more idiomatic way of describing this effect?

[tungv]

@svenanders, I'm curious about the reason why you don't want to include fetchPodcastsFn? Is it because you find it change on every render? If it is, you probably want to memoize that function or make it static (in case it doesn't have any parameters)

[robbestad]

For one - it's about clarity. When I look at the function, I want to easily understand when it should fire. If I see one id in the array, it's crystal clear. If I see that id and a bunch of functions, it becomes more muddled. I have to spend time and effort, possibly even debugging functions, to understand what's going on.
My functions don't change on runtime, so I don't know if memoizing them would matter (this one in particular is a dispatch action that fires off an epic, eventually resulting in a state change).

[ferdaber]

https://codesandbox.io/s/4xym4yn9kx

• Steps

The user accesses a route on the page, but they're not a super user, so we want to redirect them away from the page. The props.navigate is injected via a router library so we don't actually want to use window.location.assign to prevent a page reload.

Everything works!

• Intended API

I put in the dependencies correctly as in the code sandbox, but the linter is telling me that the dependency list should have props instead of props.navigate. Why is that?

A tweet with screenshots! https://twitter.com/ferdaber/status/1098966716187582464

EDIT: one valid reason this could be buggy is if navigate() is a method that relies on a bound this, in which case technically if anything inside props changes then the stuff inside this will also change.

[ThisIsOstad]

CodeSandbox: https://codesandbox.io/s/711r1zmq50

Intended API:

This hook allows you to debounce any fast changing value. The debounced value will only reflect the latest value when the useDebounce hook has not been called for the specified time period. When used in conjunction with useEffect, as we do in the recipe, you can easily ensure that expensive operations like API calls are not executed too frequently.

Steps:

The example allows you to search the Marvel Comic API and uses useDebounce to prevent API calls from being fired on every keystroke.

IMO adding "everything" we use in dependencies array is not efficient.

For example consider the useDebounce Hook. In real world usages (at least in ours), the delay will not change after the first render, but we are checking if it's changed or not on every re-render. So, in this hook, the second argument of useEffect is better to be [value] instead of [value, delay].

Please don't think shallow equality checks are extremely cheap. They can help your app when placed strategically but just making every single component pure can actually make your app slower. Tradeoffs.

I think adding everything in dependencies array, has the same (or even worse) performance issue as using pure components everywhere. Since, there are many scenarios in which we know that some of the values we're using will not change, so we shouldn't add them in dependencies array, because as @gaearon said, shallow equality checks are not very cheap and this can make our app slower.

[knpwrs]

I have some feedback about enabling this rule to automatically work on custom hooks by convention.

I can see in the source code that there is some intent to allow people to specify a regex to capture custom hooks by name:

react/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js

Lines 490 to 492 in ba708fa

	if (!isOnReactObject && options && options.additionalHooks) {
	// Allow the user to provide a regular expression which enables the lint to
	// target custom reactive hooks.

What would the React team think about an additional naming convention for custom hooks with dependency arrays? Hooks already follow the convention of being prefixed by use in order to be detected as a hook by this plugin. The convention I would propose for detecting custom hooks that rely on certain dependencies would be some sort of postfix, something like WithDeps, meaning a full custom hook name could be something like useCustomHookWithDeps. The WithDeps postfix would tell the plugin that the final array argument is one of dependencies.

The plugin could still additionally support regexes, but I think I'd see great benefit by allowing library authors to simply export their custom hooks postfixed with WithDeps rather than forcing library consumers to explicitly configure the plugin for any and all custom hooks 3rd-party or otherwise.

[timkraut]

What if fetchConfig changes? Why do you expect it to be static?

I'd be fine to add it to the dependencies array. In our particular business case, this can never happen but I guess it's a good idea to add it anyway.

My problem with this is that now the component needs to be aware about implementation details of the hook...

That's exactly the issue we have, too :/ In our implementation, we've even use an additional property to make it easier to set the body so we would have to use 2 useMemo() calls whenever we use this hook. Not sure how to overcome this limitation, yet. If you have an idea, let me know!

[asylejmani]

How can you run a useEffect which has dependencies only once without the rule complaining when passing an empty array?

Say, something like:

useEffect(() => { init({ dsn, environment}) }, [])

[gaearon]

My problem with this is that now the component needs to be aware about implementation details of the hook...

I wouldn’t say these are implementation details. It’s your API. If an object is passed we assume it can change any time.

If in practice it’s always static, you can make it an argument to a Hook factory. Like createFetch(config) that returns useFetch(). You call the factory at the top level.

I understand it’s a bit weird. We have a similar problem with the useSubscription that we’re working on. But since it’s a common problem, this might mean useMemo actually is a legit answer and people should just get used to doing it for these cases.

In practice — we’ll be looking more at this in the future. But you shouldn’t treat a potentially dynamic object as a static one because then a user can’t change it.

[gaearon]

@asylejmani You haven’t provided any details about your use cases, as requested in the top post. Why do you expect that someone can provide an answer your question?

The point of the rule is to tell you that environment and dsn can change over time and your component should handle that. So either your component is buggy (because it doesn’t handle changes to those values) or you have some unique case where it doesn’t matter (in which case you should add a lint rule ignore comment explaining why).

In both cases it’s not clear what you are asking. The rule complains that things can change, and you don’t handle that change. Without a full example, you alone can answer why you think handling it is not necessary.

[asylejmani]

@gaearon sorry for not being clearer (it always sounds clear in one's head) :) my question is more like how would you achieve componentDidMount with useEffect.

I was under the impression than an empty array does that, but the rule is telling me to always include dependencies in the array.

[sag1v]

@asylejmani I think the biggest gotcha with class life cycle methods like componentDidMount is that we tend to think of it as an isolated method, but in fact it's part of a flow.
If you reference something in componentDidMount you will most probably need to handle it in componentDidUpdate as well, or your component may get buggy.
This is what the rule is trying to fix, you need to handle values over time.

1. component mounted, do something with a prop
2. component updated (eg: prop value has changed), do something with the new prop value

Number 1 is where you put the logic in componentDidMount / useEffect body
Number 2 is where you put logic in componentDidUpdate / useEffect deps

The rule is complaining you are not doing the number 2 part of the flow

[einarq]

@gaearon sorry for not being clearer (it always sounds clear in one's head) :) my question is more like how would you achieve componentDidMount with useEffect.

I was under the impression than an empty array does that, but the rule is telling me to always include dependencies in the array.

I think me and @asylejmani are on the same page here, but I guess what you are saying @gaearon is that we are probably wrong to only run the effect on mount if we actually have dependencies.
Is that a fair statement? I guess I'm thinking that providing an empty array is sort of like saying "I know what I'm doing", but I get why you want to still have the rule in place.

Sorry for not providing a sandbox yet. I started the other night with a Create React App example, couldn't find out how to run eslint on the sandbox, and then lost the sandbox when reloading the browser without saving first (assumed CodeSandbox temp stored, my bad).
Then I had to go to bed, and haven't had time since.

In any case, I get what you are saying, and that under normal scenarios it is probably best to include those dependencies and not assume it's enough to run on mount only.

There are probably valid use-cases for that too though, but it's a bit hard to explain or come up with a good example, so I'll live with disabling the rule inline when needed.

[atomiks]

@asylejmani is your use case similar to #14920 (comment)? I don't think it's possible for the rule to understand the scenario in this case, so we'll just need to manually disable it for that type of code. In all other cases the rule works as it should.

[einarq]

Not sure if this makes sense, but one scenario that's quite common for me is something like this:

useEffect(() => {
    if(!dataIsLoaded) { // flag from redux
        loadMyData(); // redux action creator
    }
}, []);

Both of these dependencies are coming from redux. The data (in this case) should only be loaded once, and the action creator is always the same.

This is specific to redux, and your eslint rule cannot know this, so I get why it should warn. Still wondering if providing an empty array should perhaps just disable the rule? I like that the rule tells me about missing deps if I provided some but not all, or if I didn't provide any at all. Empty array means something different to me. But that might just be me :)

Thanks for all your hard work! And for making our lives as developers better :)

[asylejmani]

My use case is much simpler and I can of course add all the dependencies and it will still work the same, however I was under the impression that the rule will "warn" when you have some dependencies but missing others.

@einarq's use case is something I used a few times, for example on "componentDidMount" load the data if there is none (from redux or whatever).

I also agree that in these cases disabling the rule inline is the best choice. That case you know exactly what you're doing.

I believe my whole confusion was [] vs [some], and of course thanks @gaearon for the awesome work :)

[gaearon]

I think me and @asylejmani are on the same page here, but I guess what you are saying @gaearon is that we are probably wrong to only run the effect on mount if we actually have dependencies. Is that a fair statement?

Yes. If your component doesn't handle updates to a prop, it is usually buggy. The design of useEffect forces you to confront it. You can of course work around it, but the default is to nudge you to handle these cases. This comment explains it well: #14920 (comment).

Both of these dependencies are coming from redux. The data (in this case) should only be loaded once, and the action creator is always the same.

If it's the same then including it in the dependencies won't hurt you. I want to emphasize it — if you're sure your dependencies never change then there is no harm in listing them. However, if later it happens that they change (e.g. if a parent component passes different function depending on state), your component will handle that correctly.

Still wondering if providing an empty array should perhaps just disable the rule?

No. Providing an empty array and then wondering why some props or state is stale is literally the most common mistake.

[einarq]

Makes a ton of sense, thanks

…

On 8 Mar 2019, at 15:27, Dan Abramov ***@***.***> wrote: I think me and @asylejmani are on the same page here, but I guess what you are saying @gaearon is that we are probably wrong to only run the effect on mount if we actually have dependencies. Is that a fair statement? Yes. If your component doesn't handle updates to a prop, it is usually buggy. The design of useEffect forces you to confront it. You can of course work around it, but the default is to nudge you to handle these cases. This comment explains it well: #14920 (comment). Both of these dependencies are coming from redux. The data (in this case) should only be loaded once, and the action creator is always the same. If it's the same then including it in the dependencies won't hurt you. I want to emphasize it — if you're sure your dependencies never change then there is no harm in listing them. However, if later it happens that they change (e.g. if a parent component passes different function depending on state), your component will handle that correctly. Still wondering if providing an empty array should perhaps just disable the rule? No. Providing an empty array and then wondering why some props or state is stale is literally the most common mistake. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[gaearon]

@aweary

That would cause the side effect to occur before the actual update is committed, which might cause the side effect to trigger more often than you want.

I'm not sure what this means. Can you provide an example?

[gaearon]

We did a pass over these with @threepointone today. Here's a summary:

Fixed in the Lint Rule

Extraneous useEffect dependencies

• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @ksaldana1
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @bugzpodder (first snippet)
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @dan-lee

The rule doesn't prevent you from adding "extraneous" deps to useEffect anymore since there are legit scenarios.

Functions in the same component but defined outside of the effect

• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @jaydenseric
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @bugzpodder (second snippet)

Linter doesn't warn for cases where it's safe now, but in all other cases it gives you better suggestions (such as moving the function inside the effect, or wrapping it with useCallback).

Worth Fixing in User Code

Resetting state on props change

• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @MrLeebo

This doesn't produce lint violations anymore but the idiomatic way to reset state in response to props is different. This solution will have an extra inconsistent render so I'm not sure it's desirable.

"My non-function value is constant"

• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @jamesmosier
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @MoradiDavijani
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @billyjanitsch
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @timkraut
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @thsorens (in part)

Hooks nudge you towards correctness wherever possible. If you do specify the deps (which in some cases you can omit), we strongly recommend to include even the ones that you think won't change. Yes, in this useDebounce example the delay is unlikely to change. But it's still a bug if it does, but the Hook can't handle it. This shows up in other scenarios, too. (E.g. Hooks are much more compatible with hot reloading because every value is treated as dynamic.)

If you absolutely insist that a certain value is static, you can enforce it.
The safest way is to do so explicitly in your API:

const useFetch = createFetch({ /* config object */});
const useDebounce = createDebounce(500);
const FormInput = createInput({ rules: [emailValidator, phoneValidator] });

Then it clearly can’t change unless you put it inside render. (Which would not be idiomatic usage of your Hook.) But saying that <Slider min={50} /> can never change isn't really valid — somebody could easily change it to <Slider min={state ? 50 : 100} />. In fact, someone could do this:

let slider
if (isCelsius) {
  slider = <Slider min={0} max={100} />
} else {
  slider = <Slider min={32} max={212} />
}

If someone switches isCelsius in state, a component assuming min never changes will fail to update. It's not obvious in this case that the Slider will be the same one (but it will be because it has the same position in the tree). So this is a major footgun in terms of making changes to the code. A major point of React is that updates render just like the initial states (you can't usually tell which is which). Whether you render prop value B, or whether you go from prop value A to B — it should look and behave the same.

While this is unadvisable, in some cases, the enforcement mechanism could be a Hook that warns when the value changes (but provides the first one). At least then it's more likely to be noticed.

function useMyHook(a) {
  const initialA = usePossiblyStaleValue(a);
  // ...
}

function usePossiblyStaleValue(value) {
  const ref = useRef(value);

  if (process.env.NODE_ENV !== 'production') {
    if (ref.current !== value) { // Or your custom comparison logic
      console.error(
        'Unlike normally in React, it is not supported ' +
        'to pass dynamic values to useMyHook(). Sorry!'
      );
    }
  }

  return ref.current;
}

There may also be a legitimate case where you just can't handle an update. Such as if the lower level API doesn't support it, like a jQuery plugin or a DOM API. In that case warning is still appropriate so that the consumer of your component understands it. Alternatively, you can make a wrapper component that resets the key on incompatible updates — forcing a clean remount with fresh props. That's probably preferable for leaf components like sliders or checkboxes.

"My function value is constant"

• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @btholt
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @joelmoss
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @einarq
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @svenanders

First of all, if it's constant and hoisted to top level scope then the linter won't complain. But that doesn't help with things coming from props or context.

If it truly is constant then specifying it in deps doesn't hurt. Such as the case where a setState function inside a custom Hook gets returned to your component, and then you call it from an effect. The lint rule isn't smart enough to understand indirection like this. But on the other hand, anyone can wrap that callback later before returning, and possibly reference another prop or state inside it. Then it won’t be constant! And if you fail to handle those changes, you’ll have nasty stale prop/state bugs. So specifying it is a better default.

However, it is a misconception that function values are necessarily constant. They are more often constant in classes due to method binding, although that creates its own range of bugs. But in general, any function that closes over a value in a function component can't be considered constant. The lint rule is now smarter about telling you what to do. (Such as moving it inside the effect — the simplest fix — or wrapping it with useCallback.)

There is a problem on the opposite spectrum of this, which is where you get infinite loops (a function value always changes). We catch that in the lint rule now when possible (in the same component) and suggest a fix. But it's tricky if you pass something several levels down.

You can still wrap it in useCallback to fix the issue. Remember that technically it is valid for a function to change, and you can’t ignore this case without risking bugs. Such as onChange={shouldHandle ? handleChange : null} or rendering foo ? <Page fetch={fetchComments /> : <Page fetch={fetchArticles /> in the same spot. Or even fetchComments which closes over the parent component state. That can change. With classes, its behavior will change silently but the function reference will stay the same. So your child will miss that update — you don't really have an option outside of passing more data to the child. With function components and useCallback, the function identity itself changes — but only when necessary. So that's a useful property and not just a hindrance to avoid.

We should add a better solution for detecting infinite async loops. That should mitigate the most confusing aspect. We might add some detection for this in the future. You could also write something like this yourself:

useWarnAboutTooFrequentChanges([deps]);

This is not ideal and we’ll need to think about handling this gracefully more. I agree cases like this are pretty nasty. The fix without breaking the rule would be to make rules static, e.g. by changing the API to createTextInput(rules), and to wrap register and unregister into useCallback. Even better, remove register and unregister, and replace them with a separate context where you put dispatch alone. Then you can guarantee you’ll never have a different function identity from reading it.

I'd add that you probably want to put useMemo on the context value anyway because the provider does a lot of calculation that would be sad to repeat if no new components registered but its own parent updated. So this kind of puts the problem you might have not noticed otherwise more visible. Although I agree we need to make it even more prominent when this happens.

Ignoring function dependencies completely leads to worse bugs with function components and Hooks because they would keep seeing stale props and state if you do that. So try not to, when you can.

Reacting to Compound Value Changes

• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @aweary

It's weird to me why this example uses an effect for something that's essentially an event handler. Doing the same "log" (I assume it could be a form submit) in an event handler seems more fitting. This is especially true if we think about what happens when component unmounts. What if it unmounts right after the effect was scheduled? Things like form submission shouldn't just "not happen" in that case. So it seems like effect might be a wrong choice there.

That said you can still do what you tried — by making the fullName be setSubmittedData({firstName, lastName}) instead, and then [submittedData] is your dependency, from which you can read firstName and lastName.

Integrating with Imperative/Legacy Code

• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @atomiks
• [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 (comment) by @jamesmosier

When integrating with imperative things like jQuery plugins or raw DOM APIs, some nastiness may be expected. That said I'd still expect you to be able to consolidate the effects a bit more in that example.

---

Hope I didn't forget anyone! Let me know if I did or if something's unclear. We'll try to turn the lessons from this into some docs soon.

[trevorgithub]

@gaearon , thanks for taking the time to dive into the issues and summarizing action items into different categories. You missed my sample (#14920 (comment) by @trevorgithub) in your closing summary comment. (I certainly appreciate there was a lot of feedback from a lot of people; I think my original comment got lost in the hidden items section somewhere in the middle of the issue comments).

I'm assuming my sample would fall under 'Integrating with Imperative/Legacy Code', although maybe other categorie(s) as well?

In the case of 'Integrating with Imperative/Legacy Code' issues, it sound like there may not be a lot that can be done. In those cases, how would one ignore this warning? I guess:
// eslint-disable-line react-hooks/exhaustive-deps

[gaearon]

Sorry I missed this one.

If you receive some data via props but don’t want to use those props until some explicit change, it sounds like a correct way to model it would be to have derived state.

You’re thinking about it as “I want to ignore a change to a prop until another prop”. But you can also think about it as “My component has fetching function in the state. It’s updated from a prop when another prop changes.”

Generally derived state is not recommended but here it seems like what you want. The simplest way to implement this would be something like:

const [currentGetData, setCurrentGetData] = useState(getData);
const [prevRefreshRequest, setPrevRefreshRequest] = useState(refreshRequest);

if (prevRefreshRequest !== refreshRequest) {
  setPrevRefreshRequest(refreshRequest);
  setCurrentGetData(getData);
}

useEffect(() => {
  currentGetData(someId);
}, [currentGetData, someId]);

You’d also need to put useCallback around getData that you’re passing down.

Note that in general the pattern of passing down async functions for fetching seems shady to me. I guess in your case you use Redux so that makes sense. But if async function was defined in a parent component, it would be suspicious because you would likely have race conditions. Your effects don’t have a cleanup so how can you know when the user picks a different ID? There’s a risk of requests arriving out of order and setting the wrong state. So that’s just something to keep in mind. If data fetching is in the component itself then you can have an effect cleanup function that sets an “ignore” flag to prevent a setState from the response. (Of course moving data to an external cache is often a better solution — that’s how Suspense will work too.)

[aweary]

Doing the same "log" (I assume it could be a form submit) in an event handler seems more fitting.

@gaearon the issue I see is that doing it in the event handler means the side effect occurs before the update is committed. There isn’t a strict gauruntee that the component will successfully re-render as a result of that event, so doing it in the event handler can be premature.

For example, if I want to log that the user has successfully submitted a new search query and is viewing the results. If something goes wrong and the component throws, I wouldn’t want that log event to have occurred.

Then there’s the case where this side effect might be async so using useEffect gives you the cleanup function.

There’s also the problem of useReducer, where the value I might want to log won’t be available in the event handler. But I think that’s already on y’alls radar 🙂

In either case, the approach you recommend is likely sufficient. Store the composed state in a form where you can still access the individual values it composes.

[davidje13]

I have a convenience hook for wrapping functions with extra parameters and passing them down. It looks like this:

function useBoundCallback(fn, ...bound) {
  return useCallback((...args) => fn(...bound, ...args), [fn, ...bound]);
}

(it's actually a little more complicated because it has some extra features but the above still shows the relevant problem).

The use case is when a component needs to pass a property down to a child, and wants the child to have a way to modify that specific property. For example, consider a list where each item has an edit option. The parent object passes a value to each child, and a callback function to invoke if it wishes to edit the value. The child does not know which item ID it is showing, so the parent must modify the callback to include this parameter. This could be nested to arbitrary depth.

A simplified example of this flow is shown here: https://codesandbox.io/s/vvv36834k5 (clicking "Go!" shows a console log which includes the path of components)

---

The problem is that I get 2 linter errors with this rule:

React Hook (X) has a missing dependency: 'bound'. Either include it or remove the dependency array

React Hook (X) has a spread element in its dependency array. This means we can't statically verify whether you've passed the correct dependencies

Changing it to use a list of parameters (i.e. not using the spread operator) would destroy the memoisation, because the list is created with each invocation even if the parameters are identical.

---

Thoughts:

• is it worth showing the first error when the second also applies?
• is there any way I can disable this rule specifically for places where the spread operator is used?
• if the only use of a variable inside a function is with the spread operator, it is safe to use the spread operator in the dependencies, and since the rule is already detecting this, surely it should just allow it. I realise there are more complex cases which are harder to solve with static analysis, but this seems like an easy win for a relatively common use of spread.

[wild-lotus]

Hi @gaearon, I just got this warning, which I have not found discussed anywhere:

Accessing 'myRef.current' during the effect cleanup will likely read a different ref value because by this time React has already updated the ref. If this ref is managed by React, store 'myRef.current' in a variable inside the effect itself and refer to that variable from the cleanup function.

I find this message a bit confusing. I guess it tries to warns me that the ref current value at cleanup can be different from the value at the effect body. Right?
If that is the case, and being aware of it, is it safe/legit to ignore this warning?

My case, if interesting: CodeSandbox
Context: some custom data fetching hook. I use a counter in a ref to prevent both race conditions and updates on unmounted components.

I think I could circumvent this warning by hiding the ref read inside a function, or creating another boolean ref for the cleanup case. But I find unnecesarily verbose if I can just ignore this warning.

[gaearon]

@aweary

For example, if I want to log that the user has successfully submitted a new search query and is viewing the results. If something goes wrong and the component throws, I wouldn’t want that log event to have occurred.

Yeah that sounds like a niche use case. I think when most people want "side effects" they mean form submission itself — not the fact that you viewed a submitted form. In that case the solution I provided seems fine.

@davidje13

is it worth showing the first error when the second also applies?

Please file a new issue for proposals to change the lint rule.

is there any way I can disable this rule specifically for places where the spread operator is used?

You can always // eslint-disable-next-line react-hooks/exhaustive-deps if you think you know what you're doing.

if the only use of a variable inside a function is with the spread operator, it is safe to use the spread operator in the dependencies, and since the rule is already detecting this, surely it should just allow it.

File a new issue pls.

@CarlosGines

I find this message a bit confusing. I guess it tries to warns me that the ref current value at cleanup can be different from the value at the effect body. Right?

Yes.

If that is the case, and being aware of it, is it safe/legit to ignore this warning?

Ummm.. not if that leads to a bug. 🙂

Context: some custom data fetching hook. I use a counter in a ref to prevent both race conditions and updates on unmounted components.

Yeah maybe this use case is legit. File a new issue to discuss pls?

[gaearon]

I'm going to lock this issue since we got enough feedback and it has been incorporated.

Common questions & answers: #14920 (comment)

If you want a deep dive on useEffect and dependencies, it's here: https://overreacted.io/a-complete-guide-to-useeffect/

We'll be adding more stuff to docs soon too.

If you want to change something in the rule or aren't sure your case is legit, file a new issue.

[gaearon]

We've posted an RFC that we think provides a better solution for some of these cases.

Would appreciate your input: reactjs/rfcs#220