jsx-eslint / eslint-plugin-jsx-a11y

Static AST checker for a11y rules on JSX elements.
MIT License
3.42k stars 637 forks source link

Configuring plugin to work with polymorphic "as" prop components #923

Open stephenkoo opened 1 year ago

stephenkoo commented 1 year ago

Having trouble applying this plugin to polymorphic components that take an as or component prop which determines the html element the component will be rendered as.

For instance the commonly used Box component pattern like the one in Material UI:

<Box component="button">This is a button</Box>
<Box component="h1">This is a H1 element</Box>

The WordPress Gutenberg team seem to have the same issue.

ljharb commented 1 year ago

It's unfortunate after decades of folks realizing polymorphism is something to be avoided, that polymorphic patterns are still being used - but that said, it seems like a problem worth solving if people are insisting on doing it anyway.

What schema would you suggest?

stephenkoo commented 1 year ago

@ljharb I'm keen to learn a better way besides polymorphic components to address the use case of easily passing style-related props to any html element, if you're aware of better practice.

For example:

<Box width={[1, 1 / 2]} p={4} mb={3} bg="tomato">
  This is a Styled System box
</Box>
<Box bg='tomato' w='100%' p={4} color='white'>
  This is a Chakra UIBox
</Box>
<Text as="h1" intent="body">This looks like regular text but is a h1 element</Text>
<Text as="p" intent="title">This looks like a large title but is a p element</Text>
<Text as="button" intent="subheading" onClick={handleClick}>This looks like a subheading title but is a p but is a button</Text>
ljharb commented 1 year ago

In my experience each thing has a manually created wrapper component for it, with explicit props to control the appearance - not actually allowing anyone to pass specific colors or sizes etc, but instead, providing an explicit list of named configurations.

That said, let's keep the issue on topic :-) what schema do you suggest for a component that has a specific property to govern its "kind"?

stephenkoo commented 1 year ago

Thanks. If what you mean by schema is the JSON schema for amending the plugin, how's this:

{
  "settings": {
    "jsx-a11y": {
      "polymorphicComponents": [
        {
          "components": ["Text", "AnotherPolymorphicComponent"],
          "prop": "as",
          "values": [
            "html", // "html" is a special value that allows all html elements
            {
              "value": "fraggieBoy", // <Text as="fraggieBoy" /> is linted as a react fragment
              "type": "fragment"
            },
            {
              "value": "buttonOne", // if <Text as="buttonOne" /> is used, it will be linted as a button
              "type": "button"
            }
          ],
          "default": "div" // if no prop is provided, linted as a div. if no default is provided, a11y does not lint this component until prop is provided.
        },
        {
          "components": ["Box"],
          "prop": "component",
          // if no "values" is provided, all html elements are allowed, fragment is not allowed
        },
        {
          "components": ["DifferentPolymorphicComponent"],
          "prop": "renderAs",
          "values": ["html", "fragment"] // <DifferentPolymorphicComponent renderAs="fragment" /> is linted as a react fragment
        }
      ]
    }
  }
}

The biggest limitation which I've not good solution for is how does the linter work with conditional as props, e.g.

const App = () => {
  const isClickable = true;

  return (
    <Box as={isClickable ? "button" : "div"}>I may be a clickable button. How do I lint this?</Box>
  )
}

Would it be possible for the linter to handle this if we passed the appropriate html attributes conditionally? E.g.

const App = () => {
  const isClickable = true;

  return (
    <Box
      ...{isClickable ? {
        as: "button",
        onClick: () => { console.log("clicked") },
       ...otherButtonProps
      } : {
        as: "div"
      }
    >
      I may be a clickable button. How do I lint this?
    </Box>
  )
}

If it's the implementation detail, I'm probably going to have to take a deeper look into the a11y plugin.

khiga8 commented 1 year ago

Wanted to jump in and +1, this would be great!! This also came up in: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/issues/863.

The biggest limitation which I've not good solution for is how does the linter work with conditional as props

I agree. This might be a bit more complex to figure out, and I think it's worth kicking off support with just literal mapping and iterate on that. I believe we could get something going pretty quickly with just updating: getElementType.

khiga8 commented 1 year ago

I don't have super strong opinions, but just some initial thoughts/questions on the proposed schema:

is linted as a react fragment

Should this linter care about if something maps to a React fragment? Based on the existing rules, it seems like the linter only cares if something maps to an HTML element it recognizes, so maybe for now, we don't need to worry about explicitly mapping fragments in the configs.

"html" is a special value that allows all html elements

Given this is a "special value", could it be more appropriate as an explicit key?

          "components": ["Text", "AnotherPolymorphicComponent"],
          "prop": "as",
          "html": true,
          "values": [{"button1": "button"}]

"html" is a special value that allows all html elements

I am assuming that if this linter comes across something like: <SomeComponent as="some-custom-element"> where the tag corresponds to a custom element that isn't recognized, this would just be ignored unless there's a mapping provided in "values".

"components": ["Text", "AnotherPolymorphicComponent"],

I like that this makes it convenient for the consumer to set multiple components to the same config as I'd imagine a lot of components will have the same exact config, but I also find it a bit less-readable than if we used the component as a key like in components. Maybe that's not a big deal but just a thought. As the configs can get complex, it could be nice to provide the consumer with validation to ensure the configs aren't in a weird state like a component appearing multiple times in the configs.