React best practices | Hacker News Reader

React best practices

onoufriosm.medium.com

47 pointsonoufriosm85 comments

Translation and i18n of text is really just a function - send text in, get text out. Why would you ever put it in a hook, let alone a component?

> We managed to abstract away the logic for translation into the Translation component.

A function call. You abstracted away a function call.

> Now there is a centralized translation component where we can change any UI or business logic if needed.

A plain old JS function still centralizes i18n functionality. As for the UI logic, all the component is doing is wrapping text, therefore your component needs to support anything anyone would ever want to do a p tag. Or an h1 tag. Or a li tag. Or every other tag that can contain a text node. At some point you're just reimplementing the DOM API. What specifically is gained in going from `<p>i18n('Hello!')</p>` to `<Translation text='Hello!'/>`?

> It’s now a lot easier to test files that use translation. Before refactoring we would have to mock the useTranslation somehow because we don’t want to test its implementation. With the refactored code we only need to check that we render the Translation component and that we pass the correct props.

I can't criticize this part because I genuinely don't understand where its coming from. "We only need to check that we render the Translation component" implies that the Translation component is rendered in the test. And for the Translation component to render, the `useTranslation` hook still has to be called, so we still have to mock it, right?

I think this poster is missing the key insight that React components are indeed nothing more than functions. If anyone is reading this right now go console.log the output of your component, it's an object that describes the DOM output. Logic should not live in components, React is a rendering library. Make use of higher-order functions. Make use of a proper state management solution. Don't over-use hooks. It's really that simple.

They're definitely more than simple functions if you look at it from the the entire application point-of-view. With components you have access to context and lifecycle. Doing:

  import someFunc from 'someFunc';

  // ...
  return someFunc();

is not the same as:

  import SomeComponent from 'SomeComponent';


  // ...
  return <SomeComponent />

It all depends on how the component is written. And the function. The function could access global variables for example. The component could be pure and not use context. Neither of which you can tell just by looking at the import statement.

I guess I should clarify that I am specifically discussing i18n of text. I understand that proper i18n of apps is a much more complex topic.

I beg your pardon? Even translation of pure text (and not numbers, date, currency) relies on locale data, like plurals and genders. It's just way more elegant to have this data inside a React context instead of importing a global translator or passing in locale data whenever needed (dangerous repetition).

I genuinely don't understand this comment or the reason for writing it as a hook or a separate component in the article

Why wouldn't you just write it as a function with data and context being passed through as arguments? Isn't it much simpler to write a pure function and test that function than render a component and test it? If you simply write a function that accepts the locale, the string and context, does the translator magic and returns the translated text, would you not then just need to write a unit test simply to make sure you're getting the expected output? Why the abstraction into hooks and then further into a separate component that you then have to render?

It doesn't matter what your code is doing inside that component, you can still make it easily testable by having the core translation pipeline as pure functions.

See https://news.ycombinator.com/item?id=27031847 for more reasons.

What's "elegant" about putting it in a React Context? Outside of initial configuration, it's a stateless function. Just chuck it in a separate module and have some initialisation code. Nothing about it demands that it live in the component tree.

It is not that simple. You could use the exact module in different environments: browser, NodeJS, React Native. If you don't use context, you either need to do some very ugly module replacement at build time or duplicate your code. Also, you lose lifecycle hooks (not that important in this case though).

Coupling it to React Context makes it easier to reuse in Node? I beg your pardon? :)

If you plan to reuse it in different environments, just give the user a `setConfiguration` and a `translateFunction` function. I prefer module-level variables for holding the i18n system state, but it could also be a class-based singleton, if that's your cup of tea. I still don't see how React plays into this.

What's your programming background? This is some quite basic stuff to be honest. Have you heard of dependency injection? Context is dependency injection in React, there you go. There's quite a few .Net/Java books that would offer you better examples and clearer reasoning than myself, like this one: https://www.manning.com/books/dependency-injection-principle....

Don't talk down to people, even when you doubt them. It's not a good look :)

Context is an application of the principles behind DI, sure, but that doesn't really address my point.

Sorry if you felt talked down. I am lucky to have worked in some very big and complex React applications and will share more experience with you:

* By using a singleton you're not able to easily test different LocaleData contexts in parallel.

* By using a singleton you're not able to make your NodeJS server deal with users with different locale settings because you have only one global instance. Multiple requests arriving at the same time would corrupt each other. You could spawn a separate process for each locale but that's inconvenient.

Does that make sense?

Dependency injection in modern programming is known as "partial function application" and the poster you're replying to is correct. React Context is not at all an elegant solution, it's the opposite. It's a hard coupling to hidden global state.

It's not about loose or tight coupling, it's about things that React Context enables you do to. You're literally also hiding contextual data when using a partially-applied singleton, and you end up with code that is not safe when writing an event-looped single-threaded application (any NodeJS server implementation). Users cannot use react-helmet when doing stream-rendering, for example, because it's a singleton. See https://news.ycombinator.com/item?id=27032394.

I agree that I don't see a huge advantage to using a Translation component over a useTranslation hook, but I think the useTranslation hook can sometimes make sense over a plain old JS function, because you might want that hook to grab stuff from React context or even fetch data from the server using useEffect. It might not be feasible to have a plain old JS function with all internationalization data for all supported languages.

> Translation and i18n of text is really just a function - send text in, get text out. Why would you ever put it in a hook, let alone a component?

One concrete reason: if the language is user-configurable, you can avoid an additional argument at the call site by putting that data in a context.

Doesn't that make testing the translation and internationalization functionality that much harder though? If it's a pure function that takes a string and a locale, I can write really simple, straightforward tests that I can be confident accurately capture the breadth of the functionality.

If part of that input is now bundled up in this large globally structure, then now testing it seems like it would that much more complicated. Then, I'd need to artificially construct these structures as a part of my tests, making them less transparent to readers of the codebase.

By reducing the surface area of a widely-used interface like a translation component/hook, you massively decrease the chance of generating bugs and fewer tests would bring you way more confidence. When testing, you just need to wrap that component with a fake context that adheres to your LocaleDataContext interface (or whatever you decide to call it).

> making them less transparent to readers of the codebase

Sometimes that's exactly what you want. It's the same as hiding CSS styles behind a set of props like `<Button buttonStyle="primary" buttonSize="large">Continue</Button>`. It actually makes your codebase much safer by reducing the combination of props and values you're able to pass.

A context isn't necessarily "global state", it's "contextually shared state" hence the name. Of course, you could use it as a global state, but typically you shouldn't do that. Then again, the language locale (like a theme) is actually a compelling case for "application global" state.

From a testing perspective, you just wrap the component being tested in a context provider that provides the necessary fixtures for your test case. However, I'd also note that this is probably not an ideal function to test since all that you could possibly be testing is that react context and i8n are working as expected, which is almost certainly outside the scope of your application's testing surface.

That one is kind of odd. I use a hook for i18n but all it does is pull from a react context.

All of our tests that assert on text, assert using the i18n key. So we could re-run the suite in Arabic or Hebrew, for example, and be confident in the result.

There's no reason to mock it. It's an identity function if the translation doesn't exist.

The simplicity of the function isn't the point, the idea is that the language locale is a piece of shared state between all instances of the Translation component. With this approach, all instances of the Translation component are able to know which translation set to use without developers having to drill down a "locale" prop through the entire component hierarchy everywhere text is used (i.e. the entire application)

> "For best results we should delegate any business logic into a nested component if possible. The deeper in the components tree the logic lives the better."

Not to pile it on, but how did this make the front page?

In terms of rendering this is actually correct. If you keep all your logic super high level, then your entire component tree will re-render often.

Also, I think the author means that logic should as deep as it makes sense in the component tree which makes sense because then you can have better separation of concerns like he showed in his example.

This is going against the actual established best practice of avoiding premature optimisations. React renders are crazy cheap, and when they start getting expensive, you can just chuck a PureComponent in there and `cut off` a whole subtree from unnecessary rerendering.

There are definitely ways to mitigate excessive re-renders (Though they don't solve the problem of what happens when it actually needs to re-render), but there are also other reasons for wanting to push logic down the tree as far as possible like the one the author was conveying.

>If you keep all your logic super high level, then your entire component tree will re-render often.

This is not true, especially if you're following _actual_ best practices and using e.g. React.memo() and immutable state updates. React is explicitly built around the idea that you don't need to re-render every part of the DOM on every state update.

Right, but you do have to re-render the React DOM, and for huge apps that can get expensive, at least in theory. I've yet to work on an app big enough where we have to reach for memo/PureComponent except for very specific cases e.g. a stock price ticker

React has really turned into something else. Back in 2013, React was making waves but it was just the view layer. This was back when we were trying to replace jQuery with BackboneJS or Knockout JS or whatever. For whatever reason, MVC doesn't work in the browser so prior art goes out of the window.

The advice to take a react hook and turn it back into an HOC is bizarre. This is not a best practice.

Any post proclaiming a best practice is suspect.

There's good stuff in there though. The Action component can be a good idea. It's just an adapter, so nothing new under the sun, but it's nice to see what it looks like in React.

Some advice will definitely lead to overengineering though. Sometimes using refactor tools when needed is better than a complex abstraction.

For me, the action component advice was the worst of all. Its a "component to rule them all" which brings you closer to configuration hell.

There is nothing wrong in having separate components for "Button", "BigButton", "IconButton" and so on. It immediately tells you what the component does and the additional code is minimal.

I've been doing React for 4-5 years now. My opinion on these best practices:

1. Strongly agree. I'd add a note or two there about styled components or your favourite CSS library, and how thinking in components and the tree instead of html vs stylesheets is very useful.

2. Strongly disagree. This indeed seemed like a best practice when I was getting started, but that Action then keeps growing and growing until nobody knows exactly how it works or how to tame it. It's better to keep components small (point 1!) and focused, so let buttons be Buttons.

3. Cannot comment. I haven't worked on so many "just a CRUD app" so this might be useful for those. I can see the value of it if you have e.g. an admin panel or CMS editing a DB or similar. I would leave this repeating logic only to that though, not to the general logic of all the app.

4. Agree, but also use context to your advantage. e.g., instead of `useToggle()`, name it `useVisible()` or whatever is the appropriate name.

5. Strongly agree, it's very powerful to first write the high-level API you want by typing first the desired component tree, and then define the components themselves.

6. Yes we can try different things depending on our apps' needs, but then it's not a "React Best Practices", maybe just a useful tip? :)

I disagree with 4 as well

Maybe the author just used a poor example, but his useToggle is actually less readable to me.

The initial example with just useState is super obvious to understand what's going on in that component.

With his useToggle example, if I'm in this codebase I'm now asking myself - does toggle() fire some hidden side effect? Now I need to go check out the hook to make sure.

Well that's the goal, that you should not really care (too much) how it's exactly implemented to use it, right? It could be very simple or very complex, it could be in Redux or a simple state hook. In the case of more serious side effects like if it persists through localStorage, you read it once (hopefully through some docs!) and then you know how to use it anywhere.

I agree with you that in the author use case it's waaay too simple and the best case is to use it inline, but I believe what they meant to do (hopefully!) is use that as an example of a potentially more complex hook and how you can/should use custom hooks instead of shoving all the logic in the component.

> 2. Strongly disagree. This indeed seemed like a best practice when I was getting started, but that Action then keeps growing and growing until nobody knows exactly how it works or how to tame it. It's better to keep components small (point 1!) and focused, so let buttons be Buttons.

I'll reiterate strong disagreement here. This approach is very tempting to reduce code duplication, but in my experience you end up essentially building a "JSX" component that takes an arcane combination of props and returns some specific bit of JSX. So you end up invoking <JSX type="button" buttonColor="blue" buttonLabel="Continue" />. Why not just invoke <Button color="blue">Continue</Button>?

It's a bit light on hooks and heavy on HOCs. For people starting new React projects, HOCs are probably worth avoiding in favor of hooks when possible. That's just my opinion, but I think it's a widely shared one.

That whole User/UserRead/ReacHOC thing could be much more readable if there was just a useUser() hook instead.

I have very little React experience, but this seems a bit awkward to me:

>Therefore every app should have an action component that could look like this:

  export const Action = ({ onClick, iconProps, buttonProps, dropdownItemProps, renderProp }) => {
      if (iconProps) {
          return <Icon onClick={onClick} {...iconProps} />
      } else if (buttonProps) {
          return <Button onClick={onClick} {...buttonProps} />
      } else if (dropdownItemProps) {
          return <Dropdown.Item onClick={onClick} {...dropdownItemProps} />
      } else {
          return renderProp({ onClick });
      }
  }

Yeah, was just looking at that and raising an eyebrow.

I've only done smaller projects in React, but in 15 years of writing programs, that instinctively looks like a pretty severe code smell.

Yeah, this is the sort of situation where you want to employ some sort of polymorphism rather than a static conditional, I’d think.

In general, while some of the advice here could be useful, my experience has led me to very different opinions about structuring react.

React is very flexible and tries to avoid strong opinions, so the result is React devs have built up lots of their own opinions over the years. React code bases are almost always wildly different from each other, a blessing and a curse.

I personally wasn't a fan of any advice in this article. But again, it's all just opinion and personal experience. I didn't feel anything covered fell in the area of best practices.

I've already commented on the GP, but that's not a good thing in my experience in the long run.

Techs that quickly end up with a general "best practice" code style are a lot easier to work with, and a lot, lot, lot, lot easier to maintain.

Clojure and co are a similar vein of favoring small, focused practices and libraries over one-size-fits-all frameworks.

YMMV, but having been on both sides of this equation at companies large and small, I find the library (I.e. react) approach favorable over the framework (I.e. angular or ember) approach.

That's got nothing to do with my comment though. At all, like whoosh, you've completely missed the point of it.

My point was that it doesn't matter if it's a framework, library, language, configuration, or whatever. If the design of that thing means that the community coalesces around a particular style of usage, it's much better for everyone in the long run.

Case in point I literally only use JSX and ReactDOM.render().

Some would call this a blasphemy. Works great.

One react codebase being different from the next is pretty much the norm. I even doubt that the majority of react developers have fully transitioned from classes to hooks.

But is it typical to use neither hooks nor classes?

This seems like a good way to accidentally turn every icon into a dropdown. It's really hard to see the benefit of this Action component instead of just rendering say an `<Icon>` where you want. So much simpler to read, and the explicitness of the render method is just in your face instead of behind a `iconProps` in the props interface of `<Action>`.

I’ve seen people attempt to treat API calls like this as components and it just never quite works out well. How to populate the response to the Redux store? How to treat errors? Retries? More advanced API calls that might require some data manipulation? We’ve always found a separate API layer works best and is far easier to test.

Absolutely.. it doesn't make sense to pick a tool like React (a DOM rendering library) to solve the problem of HTTP request state management, that's just not what "component" means in the React sense.

well the problem is, that most of the time you want:

    1. show that you start a http call, by showing a loading spinner and blocking a button
    2. do the http call
    3. show the result to the user (even errors)
    4. unblock the button and hide the spinner

thus most often http requests are somehow required to be dependant on your ui. libraries like redux-saga help with that.

try { setLoading(true); const responseData = await api.some.apiCall(); updateUI(responseData); } catch (err) { showUIError(); } finally { setLoading(false); }

that was the easy part. now what do you do if two independent parts of the ui need to do something if the responseData gets updated. you either bubble downards or you need a store.

Yes, you'd probably use Redux and wrap that API call in a dispatch(). I try to stick to the design pattern of "get a response from the API call if you need the data in that function, otherwise use Redux". I've created a library that really helps with this if you're interested - https://github.com/bluedevil2k/ruffle

yeah your library looks good, unfortunatly we are using typescript and are a dotnet shop and we have our own stuff to create the store and built that into reactjs.net.

I can't see any advantage of doing it this way

You are already deciding that this component will take one of iconProps/buttonProps/dropdownItemProps, so you've already decided what type of component it is. You might as well just call <Icon />, <Button />, or <Dropdown.Item /> from the parent component and have more readable code.

Agreed, I would have a component like <CommentDeleteButton /> render a normal <Button /> by default without these different iconProps/buttonProps/dropdownItemProps.

Then give a renderButton prop to override the Button and use whatever <Icon />, <Dropdown.Item /> you want when you are using the <CommentDeleteButton />.

The crux of the problem here is overgeneralization to the point of losing meaning and scope.

The most blatant signal for this is the else case here. The else case literally calls one of the props with on of its other props passed in. You could easily ask the question "Why couldn't the consumer of this component just do that instead?"—in this else case the Action component loses meaning by breaking the single responsibility principle. It becomes so overgeneralized that literally any component could be rendered through it.

Agreed, was typing a similar comment, however yours expressed that idea better than mine.

Consider that this pattern contains N different properties that renders N different components (plus the default case), I don't know why you would want to put this into one component rather than having the consumer select the right one themselves. This even introduces the possibility of bugs - what happens if a consumer passes two sets of properties (which admittedly could be somewhat prevented by Typescript)? I don't really see the benefits of this approach for this particular example.

It feels like this is a general problem with the entire React ecosystem.

I think the benefit of a component like this is that it is a well defined interface. As long as it doesn't become a monster super-component then it seems useful.

No. This person seems not understand composition. This pattern can be useful but is poorly employed in this example. If you are interested in good practices please read Eric Elliot's Composing Software https://medium.com/javascript-scene/composing-software-the-b...

100% this person is misusing composition. Especially in the React Component sense. There seems to be some key details of React missing from the original posters' understanding, and I highly recommend anyone reading this to look into functional programming to understand what a React component is and does and how to build higher-level abstractions like the one(s) mentioned by this original poster the correct way. Higher-order functions and a real state management library are the obvious solutions to these problems

Can you elaborate on why this is a poor use of composition?

This is not at all standard practice. It should be noted that just because you read something in a post on hacker news doesn't mean it's a good idea, common practice, or even well-reasoned. OP for example is making extensive use of Hooks, which are well-known not to be the best solution to the problem of state management within React.

For example, useState leads to prop drilling, coupling, and cumbersome usage patterns.

React is a declarative rendering library, it should handle: A. What DOM elements are created? B. How are those DOM elements structured? C. What styles and attributes do those DOM elements have?

> use of Hooks, which are well-known not to be the best solution to the problem of state management within React.

I swear the introduction of Hooks threw such a monkey wrench into the ecosystem. They're just powerful enough to seem like an okay thing to use 100% of the time...

I thought we all agreed that React's ultimate destination was obscurity while it quietly became the runtime target and developers just had to return dom structures as data from functions. But they got greedy, and didn't want to go down like that, and now we have a worse version of redux instead of building on top of react-redux.

Perhaps my perception is wrong, in which case apologies and I'm happy to learn and listen.

> OP for example is making extensive use of Hooks, which are well-known not to be the best solution to the problem of state management within React.

Hooks are fine. Using React’s built in Hooks [0] for nonlocal state management is a bad idea, but using, say, react-redux and useSelector/useDispatch is a different story.

Well, if you use useState for something other than local component state, but that’s pretty expressly not using it correctly.

[0] which are fine for local, intracomponent, state management, including within groups of components that are tightly logically coupled so that implementation coupling isn’t really adding an additional burden.

Generic Action Component sounds like a premature optimization nightmare

It is indeed premature optimisation.

The single best piece of React knowledge I've found is an old idea from functional programming and MVC. Separation of components into 2 groups. Pure "view" components that contain no state and are oriented around display. And "controller" components that put everything together. The controller components make http requests, manage state, and manage which sub view is currently active. So far it has worked out well. Dan Abramov had a similar idea with presentational vs container components but missed a lot of the nuance that functional programming had brought, and thus, value.

What's gained from that?

Good question. Pure view components are incredibly easy to test and reason about. State management usually gets put into hooks that the controller component uses, which are also easier to test. You can have a nice set of unit tests for the hooks and the view components, and then a few "e2e" or integration type tests for the whole thing. Separating code into groups like this can also make it easier to find things. It also lends itself to nice reusable components.

I have found better success personally with "presentational vs container" instead of "view vs controller" as you describe it.

For example, a checkbox IMHO would be better considered a presentational component even if it manages the small bit of state it needs for it to work. Same with many of the state management that is around "visual" changes.

The point where I normally decide to split it up like you say into sub-components is just when the component becomes too complex.

Yes, many have found good success with putting a minor bit of UI state into the "mostly pure" view components. It works well until some other part of the app needs to uncheck the checkbox due to some new business requirement. This is why I usually provide a nice default "useCheckbox" hook in the same file as the pure Checkbox component.

Absolutely. The best thing I did for my React code base was to separate the entire view layer from the logic layer. That literally means I had components called e.g. UserProfileView that were stateless and unit tested, and another set of components called e.g. UserProfile that managed state.

Push all your callbacks into a separate component.

Just want to mention that the section on "wishful thinking" is usually called "optimistic updates" (if you want to learn more and googling "wishful thinking" isn't giving you great results).

I guess you misunderstood it. It has nothing to do with optimistic updates. Optimistic updates are about rendering results that you hope will happen, like clicking on a "Like" button and immediately making it bold and disabled even before the request has finished. In case it errors, you simply revert to the previous state and display some feedback to the user.

Now, wishful thinking is about making it work even if not perfect, production-ready. It could be about rendering a hard-coded "Loading..." or "Success" or "Error" string before you hand it over to your translation team. Or just calling your param names `a` and `b` before you talk to your domain experts about the proper terms. This comes with experience.

This is not something the author invented, it's a well-known technique.

Atrocious advice, I hope I never have to work in a codebase that follows these practices

Specifically what do you not like? Some of it seems iffy to me but some of it also seems obvious e.g. “use components”.

I find almost every section to be extremely distasteful.

for instance, points 2 and 3 basically suggest throwing YAGNI out the window in favor of premature abstraction. Combine it with the highly-decoupled style in point 1, and you have a nice spaghetti recipe.

I could keep going, but it looks like everyone is piling on this poor guy already.

I don't think there's a single paragraph in the article I agree with. In another comment, I picked on the parts that are the easiest to criticize in a shortish comment. The whole article screams bad OOP-style cargo cult programming, which I found particularly annoying :)

It might be obvious to you. Hiding concrete implementations behind an elegant, easy-to-use interface, is not something we see every day. I've noticed countless codebases importing HTTP clients globally, mocking a ton of modules for testing, imperatively translating messages and so on. Wrapping implementations with components/hooks allows you to future-proof your application in case you need, for ex, to access some context data, to develop for a new environment (mobile, desktop), or for easier testing as already mentioned, including snapshots. I specially like that the author talks about wrapping third-party libraries.

Another benefit of components is lifecycle. Imagine a "RelativeTime" component; you could have called library functions directly to render "20s ago", but by making it a component you could tell it to update periodically. Cool stuff.

Now, I understand that having such granular components can lead to some performance overhead if you're working on a massive application. If only there was an easy way to do AST transformations that would hoist small stateless components/hooks. Damn, AST transformations could even hoist stateful ones! I'll keep dreaming about it :)

In regards to examples such as `Fetch`, `UserRead` and `EntityActionDelete` from the post, I'm kinda on the fence because, at the same time they are uglier than hooks, they're also more performant because updates would only re-render itself and its children, and not the entire parent component. There's no right or wrong here.

I feel like “React Anti-Patterns” would be a more accurate description of most of these ideas, but most especially that Action monstrosity.

Reply on news.ycombinator.com