Performance: Update usage of stateless, functional components to class-based components when event handlers are used

[jeffcarbs]

See below for original conversation regarding "RFC: Memoize event handlers in stateless, functional components". These are the TODOs that came out of the discussion.

Functional components that need to be turned into a class (Found via grep: const handle):

• refactor(Components): use a class if there are event handlers #619 AccordionTitle
• refactor(Components): use a class if there are event handlers #619 BreadcrumbSection
• refactor(Components): use a class if there are event handlers #619 Card
• refactor(Components): use a class if there are event handlers #619 DropdownItem
• refactor(Components): use a class if there are event handlers #619 Form
• refactor(Components): use a class if there are event handlers #619 Label
• refactor(Components): use a class if there are event handlers #619 MenuItem
• refactor(Components): use a class if there are event handlers #619 SearchResult
• refactor(Components): use a class if there are event handlers #619 Step

Other places where functions are created within render (Found via grep: const on[A-Z]):

• refactor(Rating): add rating icon component #640 Rating
• breaking(Accordion): Refactor component #1375 Accordion
• Search - onMouseDown={e => e.preventDefault()} that line can actually be removed I think. The analogous updates from this PR were carried over and this line should have been removed.
• fix(Dropdown|Modal|TextArea): update refs handlers #1360 Modal - Move ref= function to handleRef callback
• fix(Dropdown|Modal|TextArea): update refs handlers #1360 Dropdown
  • Move ref= function in render to handleRef callback
  • Move ref= function in renderSearchInput to handleSearchRef callback
  • Move ref= function in renderSearchSizer to handleSizerRef callback

There may be others so feel free to update this list!

---

Original discussion below regarding "RFC: Memoize event handlers in stateless, functional components"

---

Most of the conversation around this is in reference to actual components, not stateless functions. I've googled around a bit and the closest thing I've found to a discussion on this topic is: airbnb/javascript#784. This is also more of a general React question for you guys. I've run into this in my projects and haven't really settled on a great way to handle it.

---

I think using stateless, functional components is ideal and something we should strive for. However, initializing new arrays or functions inside a render (which we do) is considered an anti-pattern for performance reasons. Consider the DropdownItem component (pasting a simplified version here):

function DropdownItem(props) {
  const { value } = props

  const handleClick = (e) => {
    if (onClick) onClick(e, value)
  }

  return (
    <ElementType {...rest} className={classes} onClick={handleClick}>
      {/** content */}
    </ElementType>
  )
}

In this case, handleClick will be a new function each time this function is called so the props for the underlying element are changing.

One idea to solve this: Doesn't work, see below.

const handleClick = _.memoize((props) => (e) => {
  const { onClick, value } = props
  if (onClick) onClick(e, value)
})

function DropdownItem(props) {
  return (
    <ElementType {...rest} className={classes} onClick={handleClick(props)}>
      {/** content */}
    </ElementType>
  )
}

This would mean that a new function would be created any time the props changed but that would be way less frequent than on every single render. Trying to get more granular (e.g. using onClick as a memoization key) would probably lead to hidden gotchas.

This actually doesn't work since props will be a new object each time so the memoization comparison won't work.

---

Definitely open to other ideas here!

[levithomason]

In the linked issue, it is noted that the linter suggesting to use a stateless component when there are instance methods is a bug. It goes on to suggest that having an event handler is actually having a stateful component, because this is the state. I can see that since the event handler is data and is attached to the component.

So, we could just use class components when there are handlers.

Because we need to pass prop values to the handler, I don't see a sane way around creating the function every time. There is a somewhat insane option IMO. Technically, you could use mutation and side-effects to avoid recreating the function, though I'm thinking it is worse than using a class component:

let value
let onClick

const handleClick = (e) => {
  if (onClick) onClick(e, value)
}

function DropdownItem(props) {
  value = props.value
  onClick = props.onClick

  return (
    <ElementType {...rest} className={classes} onClick={handleClick}>
      {/* content */}
    </ElementType>
  )
}

[jeffcarbs]

😟 that's a little rough haha. I'm also not sure it would work. handleClick is only created once and references "global" (within the scope of the file) variables value and onClick. Those variables are set to a new value on every render, so when handleClick is invoked the onClick and value will be that of the last DropdownItem rendered.

It goes on to suggest that having an event handler is actually having a stateful component, because this is the state. I can see that since the event handler is data and is attached to the component.

That makes a lot of sense to me as well. IMHO we should strive to make this library as efficient as possible so I think it's worth it to make these class-based components when there are event handlers involved.

If you agree, I'm happy to figure out which components need updating and add them as a checklist at the top of this issue.

[layershifter]

So, we could just use class components when there are handlers.

Looks like best possible solution.

[levithomason]

Agreed, classes it is