ESLint rule to detect forwardRef components without display name

[ashikmeerankutty]

Summary

Fixes #3441

• Added an eslint rule to detect forwardRef components without displayName
• Added displayName to forwardRef components without displayName

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox
- [ ] Props have proper autodocs
- [ ] Added documentation examples
- [ ] Added or updated jest tests

• Checked for breaking changes and labeled appropriately
  - [ ] Checked for accessibility including keyboard-only and screenreader modes
• A changelog entry exists and is marked appropriately

[kibanamachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[chandlerprall]

Did a quick search and re-discovered an existing eslint rule that's already part of the react/ rules: react/display-name. It's logic is more complete, and we won't have to maintain anything. It does require a handful of other EUI files will need to be updated, but it seems like a better path forward.

The rule can be disabled for src-docs via src-docs/.eslintrc.js (it flags a pretty big number of files there).

[ashikmeerankutty]

@chandlerprall Thanks for the suggestion. I tried adding react/display-name to the eslint rules. It was able to find some forwardRef components like

• https://github.com/elastic/eui/blob/master/src/components/color_picker/color_picker_swatch.tsx

But it couldn't find forwardRef components like

• https://github.com/elastic/eui/blob/master/src/components/color_picker/saturation.tsx

Am I doing something wrong?

[chandlerprall]

Ugh, good catch!

Had to dig into the eslint rule's code to figure out why, ultimately this declaration was being triggered for saturation.tsx but not color_picker_swatch.tsx https://github.com/yannickcr/eslint-plugin-react/blob/master/lib/rules/display-name.js#L84-L89

Turns out, any component containing the pattern [var|let|const variable = fn([ObjectExpression]) is considered to have a displayName generated for it. Adding this code to the EuiColorPickerSwatch function removes the display-name lint error 🤦‍♂️

  const fn = (_x: any) => null;
  const result = fn({}); // the only important line, the other two avoid introducing other errors
  console.log(result);

This appears to try and understand an old way of defining React components,

var Hello = createReactClass({
  render: function() {
    return <div>Hello {this.props.name}</div>;
  }
});
module.exports = Hello;

which does/did automatically get a displayName.

But, wow. At this point, I'd much prefer this PR's existing solution with a custom eslint rule. @thompsongl any thoughts?

(FWIW - the react/display-name rule does have a configuration flag to disable it trying to understand auto-generated display names, but toggling that flags a lot of our code base, and most of those do actually get a displayName value during transpilation)

[thompsongl]

I'd much prefer this PR's existing solution with a custom eslint rule

I'm with you on this. It may require some maintenance, but we get exactly what we need.

[ashikmeerankutty]

@chandlerprall Thanks for the review. Changes committed 👍

[chandlerprall]

jenkins test this

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3451/

[chandlerprall]

CI flagged a number of other components missing display names

17:15:57 /app/src/components/badge/badge_group/badge_group.tsx
17:15:57   45:14  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/color_picker/color_picker_swatch.tsx
17:15:57   32:14  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/color_picker/saturation.tsx
17:15:57   59:14  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/datagrid/data_grid_header_row.tsx
17:15:57   53:7  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/flex/flex_group.tsx
17:15:57   88:7  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/form/range/range_slider.tsx
17:15:57   45:14  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/form/range/range_wrapper.tsx
17:15:57   31:14  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/header/header_section/header_section_item_button.tsx
17:15:57   43:14  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/link/link.tsx
17:15:57   83:7  error  Forward ref components must use a display name  local/forward-ref
17:15:57 
17:15:57 /app/src/components/tabs/tabs.tsx
17:15:57   58:14  error  Forward ref components must use a display name  local/forward-ref

[ashikmeerankutty]

@chandlerprall The issue was with the rule. I think I missed it due to the cache. Fixed 👍

[chandlerprall]

jenkins test this

[kibanamachine]

Preview documentation changes for this PR: https://eui.elastic.co/pr_3451/

[chandlerprall]

Changes LGTM! 🎉