Account for polymorphic components in getElementType

[kendallgassner]

What

Currently, getElementType does not have a great way for testing the semantics of polymorphic components. This PR is intended to fix that.

By default, the getElementType will search for a node's attributes for the prop as. If as is set getElementType will return the value of as.

flexibility

Some libraries do not use as to set the semantics of their polymorphic components (e.g. perhaps they use html, or asChild). In order to allow for flexibility, I introduced a new setting polymorphicPropName that allows users to configure the name of the polymorphic prop.

{
  "settings": {
    "jsx-a11y": {
      "polymorphicPropName": "asChild",
      "components": {
        "Box": "p",
        "Link": "a"
      }
    }
  }
}

Questions

A couple of thoughts came up when making this pr:

1. Should I allow users to shut off the polymorphic prop search?
2. Should I allow multiple polymorphicPropNames to be passed in:

   "polymorphicPropName": ["asChild", "html"],

[ljharb]

I'd be open to being convinced, but currently I think polymorphic components are a bad practice that makes code harder to maintain and understand, and I'm not sure it's a good idea to enable that by supporting it here.

[kendallgassner]

I agree that polymorphic components can make code harder to follow but I like that they allow teams to utilize design system components without having to compromise on the semantics for their specific use case.

If a design system has a component that is almost perfect but needs a small semantic change to improve accessibility, should we make the team re-write the component? Or make the team request/implement a design system change? -- Requesting a change could take a long time and re-writing the component adds tech debt.

Though, it might be nice to implement the change on the design system side polymorphic components give teams an opportunity to make quick semantic change to a component that could prevent issues for many users.

[ljharb]

I guess I'd expect each component in the design system to be for a single semantic use case - and if someone wants a new one, the design system could provide a new wrapping component for it (with a refactor so that both components wrapped an internal one with a prop to control the semantics).

That would allow you to lint for, and clearly convey, that each component has a single semantic use. Is that approach not workable?

[kendallgassner]

My other thought...

This pattern is so widely used:

• Radix UI
• Styled components
• Carbon Design System AspectRatio.mdx
• Material UI PolymorphicComponent.ts

that it feels essential to account for polymorphic components. I think even if the setting was defaulted off, allowing for polymorphic support would benefit a wide range of jsx-a11y users.

[kendallgassner]

TODO:

• - Use getLiteralPropValue instead of getPropValue
• - Default feature off
• - Add note in readme about polymorphic component patterns

[ljharb]

Those are indeed widely-used systems - although I remain convinced that polymorphic components are a harmful pattern, being able to lint for their usage reduces harm, so let's go forward with this as an option, defaulted to false, that somehow notes in the readme that polymorphic components are a pattern to be avoided.

[kendallgassner]

I tested getElementType tests locally. I believe the other failures are due to:

• Fix failing tests in the main branch #941

[kendallgassner]

@ljharb Do you know when the test fixes will be merged?

[ljharb]

They're now merged, and I rebased this PR and cleaned up the read me a bit. Taking another look.

[codecov]

Codecov Report

Merging #945 (0799c59) into main (3d1d26d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 0799c59 differs from pull request most recent head fffb05b. Consider uploading reports for the commit fffb05b to get more accurate results

@@           Coverage Diff           @@
##             main     #945   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files         105      105           
  Lines        1640     1642    +2     
  Branches      576      578    +2     
=======================================
+ Hits         1624     1626    +2     
  Misses         16       16           

Files Changed	Coverage Δ
src/util/getElementType.js	100.00% <100.00%> (ø)

[ljharb]

This looks good; let's also add some tests in various rules that guarantee we're exercising getElementType indirectly?

[ljharb]

LGTM overall!

[kendallgassner]

@ljharb sorry to re-ping you! I just wanted to check in and see if we can move forward on this PR.

[ljharb]

thanks for the ping, sorry for the delay