
Common mistakes writing React components with hooks - loweisz
https://www.lorenzweiss.de/common_mistakes_react_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?

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

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

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

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

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

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

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

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

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

~~~
brlewis
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...](https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-
by-skipping-effects)

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

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

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

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

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

~~~
LorenzA
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

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

~~~
LorenzA
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

~~~
pg_bot
You can do it in 3 stored variables

