Common mistakes writing React components with hooks

https://www.lorenzweiss.de/common_mistakes_react_hooks/

by loweisz • 6 years ago

155 95 6 years ago

duxup4• 6 years ago

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.

zeroonetwothree• 6 years ago

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

bauerd3• 6 years ago

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

was744• 6 years ago

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

jerrycruncher• 6 years ago

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.

simonbarker872• 6 years ago

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.

nicoburns2• 6 years ago

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.

rat9988• 6 years ago

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.

bauerd3• 6 years ago

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.

kristiandupont• 6 years ago

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.

EugeneOZ• 6 years ago

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.

cryptica• 6 years ago

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

qudat2• 6 years ago

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.

tashoecraft3• 6 years ago

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.

bauerd3• 6 years ago

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.

vishnu_ks• 6 years ago

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

jfkebwjsbx• 6 years ago

A library is added to a project.

A project is added to a framework.

stblack• 6 years ago

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

luhego• 6 years ago

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.

fendy30024• 6 years ago

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.

jgwil23• 6 years ago

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.

acemarke• 6 years ago

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.

byte1918• 6 years ago

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.

LorenzA3• 6 years ago

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

pg_bot5• 6 years ago

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.

duxup4• 6 years ago

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

dgritsko• 6 years ago

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

esquevin• 6 years ago

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

flowerlad• 6 years ago

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.

fendy30024• 6 years ago

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.

jordanwallwork• 6 years ago

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

qudat2• 6 years ago

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.

jrvidal• 6 years ago

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

pg_bot5• 6 years ago

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.

kelchm• 6 years ago

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.

steinuil2• 6 years ago

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.

duxup4• 6 years ago

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.

city41• 6 years ago

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.

bulgr0z• 6 years ago

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.

brlewis5• 6 years ago

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?

vvpan3• 6 years ago

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

jgwil23• 6 years ago

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

Kiro• 6 years ago

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.

steinuil2• 6 years ago

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

tekkk3• 6 years ago

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.

brlewis5• 6 years ago

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.

tekkk3• 6 years ago

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.

cellar_door• 6 years ago

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

tekkk3• 6 years ago

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.

6gvONxR4sf7o• 6 years ago

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.

kin• 6 years ago

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.

vvpan3• 6 years ago

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.

jakkyboi• 6 years ago

Same with the empty array

evan_4• 6 years ago

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

brlewis5• 6 years ago

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.

vvpan3• 6 years ago

That's good to hear!

sickcodebruh2• 6 years ago

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

WesleyJohnson2• 6 years ago

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

ng12• 6 years ago

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.

lumens• 6 years ago

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

dimgl• 6 years ago

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

woutr_be• 6 years ago

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.

andrewingram• 6 years ago

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.

gitowiec• 6 years ago

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

Common mistakes writing React components with hooks

Related Stories