Common mistakes writing React components with hooks

[steinuil]

The first one just feels like a premature optimization. Yes calling setCount forces a rerender of that component, but unless there's lots of subcomponents inside that component, I wouldn't bother. Chances are later you'll need that state in the view, and if you have "unexpected side effects" from rerendering then that is the problem.

The other tips are fine; effects should have a single responsibility and links and buttons should be accessible.

[brlewis]

Yes. The author should replace "This is dangerous" with "This is a tiny bit sub-optimal" in the first example.

EDIT: "This is dangerous" is also the wrong label for the 2nd example. Should be "This is not the cleanest way"

EDIT2: "This is dangerous" is also the wrong label for the 3rd example. Should be "This is not the most readable". I'd also note that I'm surprised the author has seen this mistake a lot; the "solution" looks like the happy path most people would follow in the first place.

EDIT3: "This is dangerous" in the 4th example should be replaced with "This could be simplified"

EDIT4: "This is dangerous" in the 5th example should be "This makes redundant API calls". I'd also note that I'm surprised this example is even included. How could anyone miss that fetchData() is being called every time updateBreadcrumbs() is called?

[city41]

To add to this, the React docs state you should never rely on renders for side effects. In dev mode, React will even render your components twice to suss out any render relying side effect bugs. Your component should always work the same whether it's rendered once or rendered a million times.

[duxup]

Yeah I felt the same way about the first one.

Yeah we're preventing re-renders there but what are we really preventing / saving, nothing significant?

Anytime I run into a situation where I didn't like how / when a component was re-rendering, it absolutely was not because there was a random bit of state like a counter was in state that didn't need to be... usually it was just a more complex situation unfolding.

It's a good illustrative example, but not a 'common mistake' IMO.

[bulgr0z]

> The first one just feels like a premature optimization

It is in a browser context, but less with React Native which relies way more on refs. It also never hurts (imho) to explain why refs exist and why/where to use them as they can easily be abused (and often are) by devs trying to replicate OOP patterns in React.

[Kiro]

I have a really large chat app with thousands of users and I realized the other day that I accidentally re-render the root component on _every keystroke_. Still, it's not a disaster and I haven't bothered to fix it because no-one notices.

[6gvONxR4sf7o]

The router vs link one drives me crazy. I usually browse via tabs, reading breadth first rather than depth first (finish a page then read its interesting links, rather than reading links as I encounter them). Single page apps that don't let you open links/buttons in new tabs make that impossible. It frustrates the hell out of me.

[tekkk]

A recent thing which has been rubbing me the wrong way in React has been the exhaustive dependencies for useEffect and other hooks.

Sure, in the case someone would alter say the function provided as props it should be included in the dependency array. Yet in most cases, such as the example #3 in the article, this would not happen (or be even desired). Rather, if it did it would be a bug and an appropriate error would better.

So if you wanted to adhere to the strict CRA linter's exhaustive dependencies rule, you should add the fetchData function as a dependency. Or if you moved the whole function inside the useEffect, then the onSuccess. Which makes even less sense now that I've written it down.

[cellar_door]

I mean, onSuccess could change. And as you suggested, creating the extra callback is redundant.

  const fetchData = () => {
    setLoading(true);
    callApi()
      .then((fetchedData) => {
        setData(fetchedData);
        onSuccess();
      })
      .catch((err) => setError(err))
      .finally(() => setLoading(false));
  };
  useEffect(() => {
    fetchData();
  }, []);

becomes

  useEffect(() => {
    setLoading(true);
    callApi()
      .then((fetchedData) => {
        setData(fetchedData);
        onSuccess();
      })
      .catch((err) => setError(err))
      .finally(() => setLoading(false));
  }, [onSuccess]);

[brlewis]

There is no harm in adding something to the dependency array that you don't expect to ever change. The linter is your friend; make its job easier.

[zeroonetwothree]

Most of these aren’t mistakes with hooks specifically. They are just general mistakes (apply equally well to non-hooks React).

[vvpan]

I like hooks but here are some uncertainties that bother me. I am quiet new to React and hooks, but I am a senior dev, so I am wondering if any of this confuses others.

* I sometimes have to use an empty dependency array inside useEffect to run something only when component first loads. The linter yells at me, but it makes sense. I once read an article which told to put things in functions and wrap those functions in useCallback then that'd be proper, but can't imagine the value. I think most of the internet simply does empty dependency and ignores what creators of hooks suggest.

* I want to run things before the component mounts, like API calls.

* I often want my useEffect to trigger when only one of the state variables it references is updated. Again, linter screams at me and I add an exception.

* Because useEffect triggers after component mounts, and only then, it's sometimes difficult to avoid some flicker. For example I want to do something when a components prop (say "loading") changes from "true" to "false". Loading has finished, prop has been updated, component re-rendered, and only then I can trigger what I really wanted to do on that transition. I think "componentWillReceiveProps" would have solved this, but there is no functional equivalent.

Basically if there is a person who uses "useCallback" out there "correctly" I have not met them yet. My junior subordinates misuse useEffect quiet often. I often see (and use) empty dependency arrays. I see (and use) partial dependency arrays, often adding linter exceptions. Reading articles about "proper" ways of doing it sorta makes sense but it is easy to forget what the hooks authors really meant.

I think hooks is a good feature, but the authors made too many assumptions when developing them. There is a philosophy underlying them but that philosophy is somewhat incongruent with the philosophy of "I just want to get this done". React is just one tool, I have 20 more things to do every day and understanding the full philosophy of functional dependencies and when and how to properly use "useCallback" - I just do not have time, and junior devs get confused even more.

[ng12]

If the dependency array lint rule is yelling at you there's a very, very good chance your component is broken. As an example I used to see class components like this all the time:

   class Foo extends React.Component {
      componentDidMount() {
         fetchFoo(this.props.id)
            .then(foo => this.setState({foo}))
      }
      render() { ... }
   }

This component is fundamentally broken because there is no `componentDidUpdate` to re-call `fetchFoo` when `this.props.id` changes. I could easily be rendering a new id with an old id's foo. The developer is making an assumption that the initial value of `id` will never change, therefore making assumptions about how the parent component is instantiating `Foo`. Rewriting this using hooks makes this immediately obvious:

   const Foo = ({id}) => {
      const [foo, setFoo] = React.useState(null)
      React.useEfect(() => {
         fetchFoo(id).then(setFoo)
      , []) // Linter complains, and it should!
      return ...
   }

There's nothing wrong with using the empty array as your deps so long as it's actually what you want to do. If `fetchFoo()` didn't require any arguments and we only wanted to invoke it once [] would absolutely be the right dependency array. There are few cases I can think of where disabling the linter check is correct. The most common occurence in my code is if the effect uses a value not required to be fresh: e.g. something used for an optimization.

[sickcodebruh]

If you need an effect to run before the component draws, try `useLayoutEffect` instead of `useEffect`. https://reactjs.org/docs/hooks-reference.html#uselayouteffec...

[evan_]

I would ignore whoever told you not to use an empty array for the dependency argument. Change your linter rules. It's fine.

[lumens]

I found this article incredibly helpful in answering these questions: https://overreacted.io/a-complete-guide-to-useeffect/

[jakkyboi]

Same with the empty array

[duxup]

Sometimes the "don't do this" tells me more than "here's an example". Examples are great for doing the thing, but often there's reasons inside them that aren't apparent until ... you do something else.

[byte1918]

> useEffect(() => {

> fetchData();

> updateBreadcrumbs();

> }, [location.pathname]);

> There are two use cases, the "data-fetching" and "displaying breadcrumbs". Both are updated with an useEffect hook. This single useEffect hooks will run when the fetchData and updateBreadcrumbs functions or the location changes.

Is this right? Wouldn't they only update when `location.pathname` changes? Also, there should be a whole discussion on `useCallback` which is not used here and it can be quite important, plus linters would complain of missing dependencies for the useEffect.

[LorenzA]

yes seems also wrong to me. not sure what linter would complain here, that would depend a little bit what fetchData & updateBreadcrumbs really use and do.

a much bigger problem with code like this is the fetching of data gets much more complicated when the second parameter of useEffect is not just an empty array

[andrewingram]

There are a couple of potential bugs in the useEffect one.

Firstly, the two examples aren't semantically equivalent. onSuccess can change between renders, so in the "wrong" version, the version of onSuccess that'll be executed is the latest one. Whilst in the "correct" version, it'll be the one defined during the render which was in play during the initial render.

If you always want to make use of the latest onSuccess in the "correct" version, you'll probably want to look into putting it on a ref.

Another issue with the "wrong" version is that onSuccess will be called a second time if its identity changes after it's already been called once, which is quite likely if its parent re-renders and doesn't make use of useCallback.

Ultimately I think the "correct" version is indeed better, but the question of which is the correct onSuccess handler is one that always needs to be seriously considered.

[kin]

These are good points, but the examples are an oversimplification and can present problems if overlooked.

In the examples, if a function is only ever called within one hook callback, it should just be defined in that hook callback. Doing so would expose what you're missing in your dependency array.

[dimgl]

Maybe I'm being facetious, but these are remarkably simple (and obvious). Are these mistakes really that common?

[woutr_be]

I just inherited a React project that was full of these mistakes. The code was littered with "onClick" and "history.push". They used MobX because they wanted a store, even though they didn't need it. Components would often receive this store as a prop, instead of just the values it needed to render.

So yes, these mistakes are very common. There's just quite a few ways to do things, and if you don't do any research, they all have the same outcome.

[gitowiec]

I don't get some sentences written by the author of that blog post. They sound like bad grammar or illogical to me.

[bauerd]

Nitpick, but React is not a framework, it's a library. People miss this point often eg when they compare React to Angular

[simonbarker87]

I think this is an important nitpick though in your defence. I have just started learning React having become pretty competent with Angular (day job plus some hybrid apps) and I have to say that I'm struggling to understand its popularity compared to Angular.

Angular feels fully fleshed out, adheres to MVC mostly and has nice separation of html, css and the UI TS code. I come from a native coding background so this feels logical and I can jump into a new project and get my bearings pretty quickly.

React on the other hand seems to be jQuery gone mad with CSS, JS and pseudo HTML all mixed together. Ternary statements in JSX is awful to look at.

To me it seems React is solely there to solve the V in MVC and then with a host of additional libraries (of the users choice) some kind of MVC system can be cobbled together if needs be.

I'm not saying React is bad, clearly it is very popular and has it's great use cases, but I don't think it's Angular vs React.

[luhego]

Moment.js is a library, you can use in any part of your app and it doesn't define how your application is going to work. You are in charge of the flow of the application. React pretty much defines how your application is going to work from the beginning. It can call itself a "library" but by any practical means, it isn't.

[was744]

Wikipedia says it's both https://en.wikipedia.org/wiki/React_(web_framework)

[jgwil2]

React's documentation calls it a library but we don't have to. I think the reason people go back and forth on this is that there are a couple definitions of framework out there. A lot of people consider it to be a sort of continuum, where a library becomes framework-like as it adds more and more functionality. I understand that framework can sometimes connote bloat, which is probably why the React docs avoid the term. And I think it's perfectly reasonable to say that frameworks are a subset of libraries, so perhaps they're not strictly wrong to describe React as a library.

But, as others have pointed out in this thread, a much more useful distinction is where the library's code gets called in the stack. If your code is at the top of the call stack with library code below it, then it's a framework. If their code is at the top, then it's a library. In short, you call a library, a framework calls you (inversion of control). So in that sense React is most definitely a framework.

Also, I'm willing to be wrong on this one if somebody can give me a meaningful and objective reason that React should be considered a library but Angular should be considered a framework.

[vishnu_ks]

What is difference between a front-end library and a front-end framework?

[tashoecraft]

I think React is both and when you describe it as either will depend on how you're using it. If you have a website with a couple of React components here and there, then you have React the library. If you have your app setup as a SPA, with react router, redux, react forms, maybe you started the app with createReactApp, etc, then you are using React the framework. I don't see how you can argue it's not a framework when every single piece of your app is a react specific piece. It's no longer just a view layer, it's an ecosystem of modules whose common ground is to turn React into a framework.

[cryptica]

>> but React is not a framework

It's a framework which advertises itself as a library.

In reality, there is nothing library-like about bypassing the DOM for rendering and making developers use a custom programming language (JSX)... and please don't give me this tired old argument that JSX is not compulsory! Has anyone ever seen a real React application which does not use JSX? Exactly.

If 99.9999% of developers are using React as a framework, then for all general communication purposes, React is a framework.

[pg_bot]

I would argue that writing a React component with a hook is a mistake. There is usually an easier/clearer way to solve the problems that hooks are intended to solve with the existing React primitives.

[qudat]

I get what you're saying but disagree. Hooks are essential and make life much easier for the react developer, especially when using something like `react-redux` or `react-router`. Prop-drilling or HoC might seem like a better design until you actually have to work in a system that leverages them and realize it's an indirection nightmare.

[duxup]

Can you explain what you mean by that? I'm honestly a little lost on what you're saying but I am curious.

[esquevin]

Exemples would be welcome here as I've a strong belief that hooks made react components cleaner / shorter

[kelchm]

While I don't agree that writing a component with a hook is always a mistake, I have seen them used in situations where it wasn't warranted.

In the end, they are just another tool -- knowing how to use the tool appropriately is the critical part.

[fendy3002]

I haven't use hooks and don't prefer it, however I must say that there isn't alternative to hooks that is shorter in syntax, and maybe easier.