Samuel Monteiro
Samuel Monteiro

Reputation: 153

It's okay to declare other components inside a component?

Imagine that I have a List of Cards on my react application. So I will have two components, the <ListCard /> and <Card />. My doubt is that where I work, when we have something like this, we usually declare a variable inside the <ListCard /> named Card that receives a JSX, but I don't know if this is a good thing to do. Recently I faced some problems with this approach, but I didn't find anyone saying to do it or not.

Imagine that I'm not using the <Card /> anywhere in the application

The way we would declare the ListCard

const ListCard = () => {
  const cards = [
    { title: '1', id: 1 },
    { title: '2', id: 2 },
  ];

  const Card = ({ title }) => <div>{title}</div>;

  return (
    <div>
      <h1>List of Cards</h1>
      {cards.map(card => (
        <Card title={card.title} key={card.id} />
      ))}
    </div>
  );
};

I want to know if it's better to declare outside of the ListCard, like this.

const Card = ({ title }) => <div>{title}</div>;

const ListCard = () => {
  const cards = [
    { title: '1', id: 1 },
    { title: '2', id: 2 },
  ];

  return (
    <div>
      <h1>List of Cards</h1>
      {cards.map(card => (
        <Card title={card.title} key={card.id} />
      ))}
    </div>
  );
};

Upvotes: 15

Views: 15518

Answers (4)

Mark Johnson
Mark Johnson

Reputation: 93

The React docs, which should be a good authority regarding this, explicitly advises against nested component definitions.

The main issue is with hooks. Taking the example from the second link:

import { useState } from 'react';

export default function MyComponent() {
  const [counter, setCounter] = useState(0);

  function MyTextField() {
    const [text, setText] = useState('');

    return (
      <input
        value={text}
        onChange={e => setText(e.target.value)}
      />
    );
  }

  return (
    <>
      <MyTextField />
      <button onClick={() => {
        setCounter(counter + 1)
      }}>Clicked {counter} times</button>
    </>
  );
}

Whatever state is held by MyTextField gets lost when MyComponent rerenders, because MyTextField is redefined every render. From React's perspective, the MyTextField from a previous render is an entirely different thing from the MyTextField of the current render, and thus won't reassociate their states.


Personally however, I agree with the other comment that it might be fine to "declare" component definitions inside a component, especially if they are hook-less. E.g. refactoring an incredibly long ternary rendering into a function with if-returns

Upvotes: 4

Sharcoux
Sharcoux

Reputation: 6075

Contrary to what is said in the other answers, it might be perfectly fine to declare components inside a component. It can even be quite useful in some situations where you need to access functions from the parent scope, and when rendering each item would be costly. The only thing is that you need to understand what you're doing, be careful, and do it correctly. But you will learn that, with React hooks, any approach always need you to have a good understanding of the render process.

In your case, to prevent remounting the component on every render, you need to keep the same reference on the child component you are creating. For that, we use the useMemo hook. Be careful, because each time a dependency of that hook updates, your reference will change and so the children will remount. There are ways to work around this and always keep the same reference, but that might be a bit off-topic.

Here is a common example:

const List = ({ items, onItemSelected }) => {

  // If you don't use useMemo, your component will be recreated every time and
  // React will destroy and recreate every item at every render.
  const Item = React.useMemo(() => ({ index, item }) => {
    const onClick = React.useCallback(() => onItemSelected(index), [index])
    return <li onClick={onClick}>{item}</li>
  }, [onItemSelected])

  return (<ul>
    {items.map((item, index) => <Item key={index} index={index} item={item} />)}
  </ul>)
}

You can find a complete working example here

Here is the full code with some comments you might want to pay attention to.

import React from "react";

const items = ["apple", "banana", "orange"];

export default function App() {
  const [selected, setSelected] = React.useState(-1);
  const onItemSelected = React.useCallback((index) => setSelected(index), []);
  return (
    <div>
      {/* Add some logs and see what happens if you add "selected={selected}" to the props of List */}
      <List items={items} onItemSelected={onItemSelected} />
      <p>
        You pressed <b>{items[selected]}</b>
      </p>
    </div>
  );
}

// We used React.memo so the list won't rerender when the parent state changes
// This is a good optimization, but nothing mandatory, here
const List = React.memo(({ items, onItemSelected }) => {
  // If you don't use useMemo, your component will be recreated every time and
  // React will destroy and recreate every item at every render.
  const Item = React.useMemo(() => {
    // We use this syntax only for eslint to detect that this is a component.
    // There is nothing mandatory, here, neither.
    const Component = ({ index, item }) => {
      // The main interess here, is that "onItemSelected" is not a prop anymore.
      // This way, we can make onClick a callback that won't change between renders
      const onClick = React.useCallback(() => onItemSelected(index), [index]);
      return <li onClick={onClick}>{item}</li>;
    };
    Component.displayName = "Item";
    return Component;
    // be very carefull with what you put here. The component should not depend on things
    // That might change often, because every items will be destroyed and recreated when they change
    // Try to add "selected" in this dependency list and see the impacts on rerender
  }, [onItemSelected]);

  return (
    <ul>
      {items.map((item, index) => (
        <Item key={index} index={index} item={item} />
      ))}
    </ul>
  );
});

Upvotes: 2

Brian Thompson
Brian Thompson

Reputation: 14355

In a functional component, every variable gets destroyed and recreated again every render. This is what makes the useState hook so valuable, as it is smart enough to recreate it's variables with the previous or updated values.

By declaring a component inside another component, you are not only re-rendering both components, but completely redeclaring one. This won't be very performant, especially if the component is more complex.

So the answer to your question is always declare it separate. It would work declared inside, but there's no benefits from it, only drawbacks.

Upvotes: 8

don_jon
don_jon

Reputation: 421

It is totally fine to declare another Component in the same file, but declaring it inside another one's function would be inefficient. Imagine your app 'recreating' the second Component during each render.

So, feel free to declare it in the same file, but don't do it inside another Component's function.

Upvotes: 22

Related Questions