Common mistakes writing React components with hooks | Hacker News Reader

Common mistakes writing React components with hooks

lorenzweiss.de

155 pointsloweisz95 comments

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.

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.

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.

> 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.

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?

Yeah, there are gotchas and issues with hooks and some of them are quiet common but this article does not cover those. Odd selection of "mistakes".

Well, example 2 is legitimately dangerous if your app is required to be accessible.

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.

Shouldn't be a big deal if it's properly split into subcomponents and their props don't change with every rerender as well.

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.

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.

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.

Well, I disagree. As I said in my other reply - if the intention is that the prop is immutable there should be an error instead of very strange recomputation of the useEffect hook.

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]);

Sure it could but if the intention of the developer is to run the API call only once per mount, never otherwise, it would be more appropriate to throw an error instead of just adding pointless dependencies. It just seems like a dumb overhead which goes against the actual goals of the developer.

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

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.

Same with the empty array

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

That runs the risk of someone less experienced introducing a bug later when a new dependency is introduced. I think it's better to simply do whatever the linter says.

There's also a risk that someone less experienced hits themselves in the thumb with a hammer, and then hits themselves between the eyes with the claw, and then drops the hammer on their foot.

Are you saying that writing code in a way that defends against future mistakes is a waste of time? It's hard to see the content of your comment through the sarcasm.

I do think that's important. I practice that by writing readable, well-formatted code, commenting liberally, and avoiding "too-clever" hacks. I don't think using a well-documented feature for the purpose it was intended violates any of these.

Is the well-documented feature the dependency array or the ability to suppress linter rules? The note at the bottom of the documentation section for the dependency array recommends using the linter rule: https://reactjs.org/docs/hooks-effect.html#tip-optimizing-pe...

That's good to hear!

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

How do you handle a useEffect that relies on state, but you don't want it to run on first render? I usually have a separate state that is "firstLoad" and toggle that in the effect and exit out, then its free to run on successive invocations. But typing this out - it makes me the think the dependent state itself should be driving if it runs or not...

I don't think there's a way around tracking that `firstLoad` value. A `ref` is probably a better choice than a separate state value because you don't need to render when the component becomes aware that you're past the first render. You could do it in a simple hook:

  function useFirstRenderComplete() {
    const renderCountRef = useRef(0);

    useEffect(() => {
      if (renderCountRef.current) {
        return;
      }

      renderCountRef.current += 1;
    }, []);

    return renderCountRef.current !== 0;
  }

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.

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

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.

> 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.

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

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.

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.

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

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.

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

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

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

Yeah, I think that prior to the public release of the Context api (16.3) and Hooks (16.6), one could more easily make the argument that "it's just a display library."

Providing convenient mechanisms for managing high-level/application state moves them squarely into the framework category, imo.

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.

To me, the killer feature of React is how easy it is to mix with standard "vanilla" JS libraries. The "React" version of a library is usually a fairly trivial wrapper around the "normal" version. In contrast, integrating with Angular is a lot more involved and is in effect its own ecosystem.

The other thing I can't stand about Angular is that it puts its proprietary templating language "in control" of components. Which means if you want to do something more complicated that isn't implemented in the templating language, then you just can't (or you end up fighting the framework). With React, it's as if the equivalent of Angular's controllers (components? - it's been a while since I used Angular) were in control. The templating is only for templating.

FWIW, I agree on the ternary operator issue. Although IMO this is actually a deficiency in JavaScript rather than in React/JSX: If if-else and switch in JavaScript were expressions then this problem would disappear completely.

Can you provide some examples? My experience is that almost any library you end up using is built specifically for React and not vanilla.

Redux can be used in projects unrelated to React.

I have used react-sortablejs and react-leaflet in the past. I don't have any experience actually wrapping existing libraries myself though.

https://github.com/SortableJS/react-sortablejs

https://react-leaflet.js.org/

Compare https://github.com/nhagen/react-intercom/blob/master/src/ind...

with https://github.com/PatrickJS/angular-intercom/blob/master/an...

And that's the small one. There's also https://github.com/CaliStyle/ng-intercom

As another example, consider HTTP. Angular provides its own special HTTP class. With React I just use the standard Fetch API.

I don't see why the CSS is mixed with the rest. It clearly isn't. Mixing JS and HTML doesn't seem problematic to me.

Depends if you use CSS modules or the styled component library.

I've mixed plenty of html and js over the years and even html and php and it's nice to not have to anymore. Something about JSX rubs me the wrong way.

With React Native css is commonly mixed with the JS/TS, all in the component.

Correct, React is essentially a tree diffing & reconciliation library tailored for the DOM. As such its foremost concern is to provide a declarative API for manipulating the UI tree.

How data flows into that tree is not its concern. This is why there's a wide variety of state management libraries available. Empirically I agree teams often get state management wrong, because it requires a different skillset than developing reusable UI components.

Frameworks like Angular make a lot of hard choices outside the view layer for you.

MVC is a popular pattern but it's not the only way to do UI. I don't personally care for it and I am happy that React doesn't force it on me.

As for mixing HTML, JS and CSS, I consider that a feature. Why do you want those separated? The reason for me to group code in a file, a folder or at a higher level, in a repository, is that I create a little mental context for what I am going to work on and I want the structure to enable that as much as possible. Now, when I am focusing on some feature it is much more likely that I will be switching back and forth between the HTML, CSS and JS of a component than switching between the CSS of one component to another.

It happens, of course, that I get into a mode where I want to edit many CSS files in one go for whatever reason, it's just much less common and thus I don't want to optimize for that case.

> Why do you want those separated?

You don't, there was a time when separation of concerns got mistranslated into separation of technologies and it became an almost religious mantra. But if HTML, CSS, and JS all combined to create a single black box component, that is a single concern, separating the technologies for separations sake only serves to reduce the reasonably of the black box. There are still those that where taught in that time, that separation of technologies and separation of concerns where the same thing, but they are not and that is well evident in that react component are easy to reason about.

There's nothing wrong with ternary operators in my opinion - maybe it's just a subjective thing.

Most interesting thing in React for me is the ability to pass an element as an argument to another component - it's impossible in Angular. I’m not sure there is a lot of real-life cases for this feature, but still, I’m impressed by this level of flexibility.

React isn't just V of MVC, it's VC at least, letting you write your models wherever you want (pure js functions? classes - go ahead).

Some advantages of React are the weak points simultaneously - ngModel, ngIf, ngFor - they are removing a lot of boilerplate you have to write in React.

>> 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.

> Has anyone ever seen a real React application which does not use JSX?

I've built one without using JSX and I think it's superior. The problem is the tooling and type support just isn't there so going against the grain here is pretty much asking to be burnt.

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.

>If you have you're 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

react-router, redux etc. are all different libraries. Point still stands: You have to pick and assemble these different parts yourself. And then still all those libraries don't call into your code, but your code includes and wires them up.

Whether or not they are different libraries doesn't matter. Once you've done any real development with them good luck trying to take out your small, simple library and replace it with another. Why are so many React devs so against the word framework, it's like they treat it how many treat monolith "It's a bad word we can't be compared against, even when you can't tell us apart"

Well I sometimes use react to replace a component, such as develop a button which needs to be clicked twice to submit just by simple state management.

Though I don't know if it still can be considered as framework or not.

Then it's a library, which I stated in my first comment "If you have a website with a couple of React components here and there, then you have React the library.". I'm mostly just trying to point out was seems to be an aversion to the word framework by the react community, when in most cases it is. It's not bad to be a framework, it has loads of benefits, but it's not a bad word that needs to be reviled at all times.

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

A library is added to a project.

A project is added to a framework.

I agree with this definition.

So how is React a library? You can't just utilize part of React within an existing Angular app. I've built shims between the two frameworks and you can't just use React inside of Angular. When you utilize a React component inside of Angular, everything Angular about the application goes out the window and it becomes a mini React app starting from that component, just with the data originating from an Angular app.

The definition of a framework boils down to inversion of control, right? You define your application, and then it is run within the React "framework" context, which calls various predefined methods/functions.

All of the lifecycle methods in React are a good indicator, to me, that it is indeed a framework. You define these functions, and they are called by the framework. Inversion of control.

Your code calls a library.

A framework calls your code.

Really great explanation here: https://stackoverflow.com/posts/15600924/revisions

Edit: another really nice exposé here: The Difference Between a Framework and a Library — https://www.freecodecamp.org/news/the-difference-between-a-f...

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.

Kinda disagree. Though I usually use it as a full page form, it's easy to use it as single component and use it via javascript natively.

Let's say that if I want my own version of select2, I can develop it with react, though not easy.

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.

React _is_ a framework, because:

- You hand it your code and it calls your code when it wants to

- Your code must conform to React's expectations.

React is _not_ a framework, because:

- It only focuses on one thing: defining a tree of UI components. It doesn't include anything for HTTP requests, module definitions, generating expected file structures, or any of the other stuff you'd see in Angular and Ember.

- You are responsible for initializing React in your app, and you can use it in a range of situations, from a full-bore SPA to adding some interactive widgets to an existing page.

All those are true simultaneously.

They are true simultaneously only if we allow two definitions of "framework" to exist simultaneously. As someone who prefers precise language, I think there should only be one such definition, namely the inversion of control definition.

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.

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

I read it as "I don't like hooks."

I wondered the same but the text was also confusing so I thought maybe there was something else there.

At least for me hooks dramatically clean up the code.

Outside of error boundaries (I think those still have to be classes) any new component for me is a function component and if it needs state, has hooks.

Granted I still maintain a lot of class components back from before the days of hooks.

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

For one thing you may not use the ref attribute on function components because they don’t have instances. That means the component can't have a .focus() method for example.

For simple components hooks may be fine. For more complex ones I prefer classes.

I believe the useRef hook is used for this problem:

https://reactjs.org/docs/hooks-reference.html#useref

The example even uses .focus()

That's just not true: https://reactjs.org/docs/forwarding-refs.html

Maybe I misunderstood, but isn't this what `React.forwardRef` is for?

along with useImperativeHandle for writing your own imperative functions like focus() or whatever

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.

Yeah I wasn't sold on hooks at first, mainly because I was having to re-think stuff that I already knew, but man now I'm used to them I'm so much more productive and I absolutely love it

I echo this statement. I use Django a lot for my backends and I was very, very against moving to class-based views. I'm not sure why, I love OOP, but I was so used to function-based views. I finally made the switch (well, I use both when appropriate) and I love them.

During this time, I started playing around with React and I thought, I learned my lesson, it's class-based components all the way. Yet again, the community started going to "other" way with function-based components and I was steadfast in my attachment to classes - refusing to budge.

Of course, I eventually made the switch and wish I had done so sooner. Once you get the hang of hooks, they're so much easier to reason about and, as you said, make me much more productive.

That's not without reason, javascript class is infamous for being weird with 'this' gotchas, and class is 2nd class citizen there (not as good / powerful as one in java). Meanwhile function based view is neat.

However without hooks, only class based component has side effects and lifecycle, so you like it, then made switch to hooks because it's neat.

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.

Pardon my ignorance, but is prop drilling a problem once you've bought into Redux or similar?

No it provides helper functions that inject props into your component.

I would go a step further and say that React Components should not manage their state internally. (this.state was a mistake to add to the project) You can create every single UI in the world using pure functional components. They are easier to understand since they act like every other function, and they are simpler to test.

Keep your state externally and just grab what you need on each subcomponent with the `connect` function from `react-redux`. Easy peasy front-end development.

not sure i understand you correctly, but keeping all state external just clutters up the store i feel. if the state is not used anywhere else and you also don't need to change it externally i would not move state out of the component. also if your state is always outside of your component they will only be reusable if you hook up the store in all your projects the same way

Nah you can get reusability by creating shared components that you just pass props into.

For example, you can have a date picker that just accepts variables to display the current state and parent component that reads data from the store and passes it into the date picker component.

This method is also great for debugging, since you can just replay the state transitions over again if there is an application error. If the component holds the state, then it can be cumbersome to reproduce the error.

a date picker would be super annoying to use if you would have all the state in the store. that are probably 40 variables. assume you open the date picker can can move threw the tabs of the month and you are not interested in changing this from outside.

if you have no state in the component that would not be possible. you can also not wrap this with an other component that can hold this state because you don't want components with state. that means all behaviour that requires state would have the state in the store this will just be crazy

You can do it in 3 stored variables

Redux is global state. Adding all local state to a global store will result in a complete mess for any non-trivial app.

Maybe if you're writing it ;)

Most of the time it mirrors the structure of a file system which is the exact way (almost) every program is written.

As with most software projects, keeping organized is like 80% of the battle.

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.

Reply on news.ycombinator.com