6

Is this an anti-pattern?

export function Todo() {
    ...

    const renderItem = (item) => (
        item.done
            ? <strike>{item.text}</strike>
            : <span>{item.text}</span>
    );

    return (
        <ul>
            {items.map((item) => <li>renderItems(item)</li>)}
        </ul>
    );
}

What's the difference between rendering items like that, as compared to making an Item component inside Todo, such as this:

export function Todo() {
    ...

    const Item = (props) => (
        props.item.done
            ? <strike>{item.text}</strike>
            : <span>{item.text}</span>
    );

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

EDIT:

What about creating components/render functions locally, that are called once?

export function SomeForm(props) {
    const renderButton = (isComplete) => (
        isComplete
            ? <button>Submit</button>
            : <button disabled>Please complete</button>
    );

    return (
        <form>
            <input />
            {renderButton(props.isDone)}
        </form>
    );
}
wrahim
  • 1,158
  • 4
  • 16
  • 33
  • [Losing state between renders if component is defined in another component](https://stackoverflow.com/a/66571922/2873538) – Ajeet Shah May 09 '21 at 14:16
  • Does this answer your question? [Is it an anti-pattern to define a function component inside the render() function?](https://stackoverflow.com/questions/52372040/is-it-an-anti-pattern-to-define-a-function-component-inside-the-render-functio) – Emile Bergeron Jan 18 '22 at 06:17

1 Answers1

12

Beforehand lets fix the examples into a valid code:

// #1
export function Todo({items}) {
  const renderItem = (item) =>
    item.done ? <strike>{item.text}</strike> : <span>{item.text}</span>;

  return (
    <ul>
      {items.map((item) => (
        <li key={item.id}>{renderItems(item)}</li>
      ))}
    </ul>
  );
}

// #2
export function Todo({items}) {
  const Item = (props) =>
    props.item.done ? <strike>{item.text}</strike> : <span>{item.text}</span>;

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

Back to the question, Yes those are anti-patterns.

Why it's an anti-pattern?

In both examples, you will rerender the item even though there wasn't any visual change.

The reason for it because on every render, you recreate both function (renderItem) and function component (Item).

What you want

Instead, you want to let React do the Reconciliation process, for it you need render a static tree as much as possible.

The easiest solution is just move the function/function component, to the outer scope or inlining the logic into the tree itself.

const renderItem = (item) => (...)
const Item = (props) => (...)

export function Todo({ items }) {
  return (
    <ul>
      {items.map((item) => (
        <li key={item.id}>
          (item.done ? <strike>{item.text}</strike>:<span>{item.text}</span>)
        </li>
      ))}
    </ul>
  );
}

What's the difference between rendering items like that

renderItem is just a function returning JSX, Item is a React component, therefore its state is registered to the React tree ("just a function" can't hold its own state).

Dennis Vash
  • 50,196
  • 9
  • 100
  • 118
  • Thanks for clarifying the difference between "just a function" and a react component! I understand that this is especially inefficient when rendering a list of sorts, but what if I am rendering something just once. Is creating a "render" function or a local component still an anti-pattern then? I added (a rather contrived) example in the edit. – wrahim May 12 '21 at 21:26
  • 1
    Your edits are the same examples, still anti pattern – Dennis Vash May 13 '21 at 06:54