4

I've started seeing some of my team writing the following code which made me question if we're doing things the right way as I hadn't seen it written like this before.

import * as React from "react";
import "./styles.css";

function UsualExample() {
  return <h1>This is the standard example…</h1>
}

export default function App() {
  const CustomComponent = (): JSX.Element => <h1>But can I do this?</h1>
  return (
    <div className="App">
      <UsualExample />
      <CustomComponent />
    </div>
  );
}

It seems to render fine and I can't see any immediate adverse effects but is there some fundamental reason why we shouldn't be defining CustomComponent functional component from within another component?

CodeSandbox Example: https://codesandbox.io/s/dreamy-mestorf-6lvtd?file=/src/App.tsx:0-342

Justyn
  • 1,442
  • 12
  • 27

2 Answers2

10

This is not a good idea. Every time App rerenders it will make a brand new definition for CustomComponent. It has the same functionality, but since it's a different reference, react will need to unmount the old one and remount the new one. So you will be forcing react to do extra work on every render, and you'll also be resetting any state inside CustomComponent.

Instead, components should be declared on their own, not inside of rendering, so that they are created just once and then reused. If necessary, you can have the component accept props to customize its behavior:

const CustomComponent = (): JSX.Element => <h1>But can I do this?</h1>

export default function App() {
  return (
    <div className="App">
      <UsualExample />
      <CustomComponent />
    </div>
  );
}

On occasion, you may be doing something repetitive inside a single component and want to simplify the code by having a helper function. That's ok, but then you'll need to call it as a function, not render it as a component.

export default function App() {
  const customCode = (): JSX.Element => <h1>But can I do this?</h1>
  return (
    <div className="App">
      {customCode()}
      <UsualExample />
      {customCode()}
    </div>
  );
}

With this approach, react will be comparing an <h1> with an <h1>, and so it does not need to remount it.

Nicholas Tower
  • 72,740
  • 7
  • 86
  • 98
  • Thanks for this answer, this makes sense and also covers the detail as to why it is a bad idea regarding rendering performance. We often use the helper function approach as you mention above so will stick with that or as you and Adam both mentioned, just abstract out into another function if we want a component. – Justyn May 27 '20 at 16:28
  • "helper functions" suffer from the **exact same issue** as components. They're only necessary to close over some variable that's available in the scope of the function. Just pass these variables as arguments to the helper function and extract your helper function from your component. It's bad perf, bad design, bad everything to declare functions (without using `useCallback`) inside of components. – Adam Jenkins May 27 '20 at 16:42
  • `"helper functions" suffer from the exact same issue as components` while there are certainly arguments against helper functions, they do *not* suffer from having different element types, and so they will not cause the reconciler to mistakenly think the elements have changed. – Nicholas Tower May 27 '20 at 17:12
  • @NicholasTower - in your example above, change `customCode()` to `` and see what happens - it's a very very very thin line. What you've done is created a component (it returns JSX right?), but called it something different (it's not) so now it can't benefit from Reacts many numerous performance optimizations. – Adam Jenkins May 27 '20 at 17:34
  • Yes, i'm quite aware that `` and `customCode()` have different results. That was pretty much exactly the point of my answer. – Nicholas Tower May 27 '20 at 17:37
  • @NicholasTower - I wasn't trying to imply you didn't know what you were talking about, but "helper functions" are often just used to return bits of JSX. Any function that returns JSX **is a component** whether it's called like a function `customCode()` or it's called with the special syntax `` – Adam Jenkins May 27 '20 at 17:37
  • Ah, i see, so you're concerned about the terminology, not the functionality. Ok, i'm fine with calling it a component when you call the function. But you are aware that the behavior is different, yes? – Nicholas Tower May 27 '20 at 17:40
  • @NicholasTower - the behavior is not different when you declare "helper functions" in the body of the component https://stackblitz.com/edit/react-tavv9k, unless you're counting remounting/loss of state as different behavior (which most developers are confused by, hence this entire SO question). – Adam Jenkins May 27 '20 at 17:41
  • `unless you're counting remounting/loss of state as different behavior` That's exactly what i said the difference is, multiple times. So i'm glad we agree. – Nicholas Tower May 27 '20 at 17:44
  • @NicholasTower my ultimate point is the pattern suggested in your answer is also an anti-pattern in react - calling a functional component as a function (by adding () instead of using JSX syntax) and pretending it's not a component. – Adam Jenkins May 27 '20 at 17:45
5

It's not only a bad idea, it's a horrible idea.

It’s a horrible idea because react components have a life cycle. They get mounted, and they can maintain state either in themselves or somewhere lower in their tree (the things they render). But this can’t happen if the component definition changes on every render of a parent. You’ll end up seeing weird bugs like state getting seemingly reset at odd times.

The only reason why you want to declare a component inside of another one is to close over a prop (maybe some state too, perhaps) that you want to capture in a child component - here's the trick - pass it as a prop to a new component and you can then declare the new component OUTSIDE.

Turn this:

function UsualExample() {
  return <h1>This is the standard example…</h1>
}

export default function App({someProp}) {
  // the only reason you're declaring it in here is because this component needs "something" that's available in <App/> - in this case, it's someProp
  const CustomComponent = (): JSX.Element => <h1>I'm rendering {someProp}</h1>
  return (
    <div className="App">
      <UsualExample />
      <CustomComponent />
    </div>
  );
}

Into this:

function UsualExample() {
  return <h1>This is the standard example…</h1>
}

const CustomComponent = ({someProp}) => <h1>I'm rendering {someProp}></h1>

export default function App({someProp}) {
  return (
    <div className="App">
      <UsualExample />
      { /* but all you have to do is pass it as a prop and now you can declare your custom component outside */ }
      <CustomComponent someProp={someProp} />
    </div>
  );
}
Adam Jenkins
  • 51,445
  • 11
  • 72
  • 100
  • 5
    "It's not only a bad idea, it's a horrible idea." - should explain why. "You haven't yet discovered composition (it's ok, it takes a while, you'll get there)." - This is a bit patronizing and unnecessary, especially since OP stated "I've started seeing some of my team writing the following code" and is inquiring about an observed pattern. Not all languages or frameworks have optimization concerns with nested function declaration, so it's not a foregone conclusion. – Damon May 27 '20 at 20:16
  • While not saying it's a good idea. There are other reasons someone would be tempted to declare a component inside another component that doesn't involve closures. As @Damon said, not all languages have issues with nested function declaration and nested function can be a great way to make your code more organised especially when the nesting aligns with the visibility. i.e. if a block of code you want to separate is only used inside of function A() then putting it in a nested function can be considered more expressive, especially for other developers working on the code. – anavarro9731 Apr 02 '23 at 14:07
  • @anavarro9731 this isn’t an issue with nested function declaration. JS as a language and a react as a framework have no issues with nested function declaration. But nested **component** declaration is non-sensical. – Adam Jenkins Apr 02 '23 at 14:18
  • 1
    @Damon - very fair criticism. Answer has been updated. – Adam Jenkins Apr 02 '23 at 14:22