[spike] Allow custom component to be mapped to element type

[khiga8]

DEPENDS ON #282!
cc: @github/accessibility-reviewers

Context

In #282, we introduced a rule that lints against use of generic text on links.

However, this lint rule is mostly useless for GitHub developers since GitHub developers are likely to use the Primer React component, <Link> rather than the HTML <a>. We want the lint rules we write to run on Primer components too.

This PR allows for custom components to be mapped to an HTML element in the eslint configuration.

settings: {
  github: {
    components: {
      Link: {
        default: 'a'
      }
    }
  }
}

The above example will allow all Link component to be mapped to an anchor element by default. This may make sense when the tag is fixed.

However, one layer of complexity is how Primer React components also support an as type. To accommodate for that, the configuration should allow a mapping of a prop value of a component to an element type.

settings: {
  github: {
    components: {
      Link: {
        props: { as: { undefined: 'a', 'a': 'a', 'button': 'button'} }
      }
    }
  }
}

This above setting specifies no default. It instead says that if there's a prop type of as and if that prop is undefined OR is defined and set to a, the component should be mapped to an a tag. This ensures that if as is set to some other value that isn't configured in props like summary, we keep the raw type of Link (rather than defaulting to anything).

For now this config will only support one type of prop in the props section. I decided this might be better because combinations of props can lead to conflicts and add complexity. Also I don't see a need for supporting complicated prop specification (but let me know if there is a need for it)

Note to reviewer

I'm open to any feedback on how this config should be shaped.
I'm thinking that we can introduce a preset to eslint-plugin-primer-react which contains configuration for this plugin for Primer React components.

[khiga8]

This eslint custom component mapping approach was inspired by, jsx-a11y. however, jsx-a11y only supports 1:1 mapping whereas we want to be able to associate a prop with a component.

(read more about jsx-a11y custom component mapping)

[khiga8]

I thought tests/**/*.js will run all tests including ones in nested folder but it only ran the nested tests. Therefore I had to specify tests/ too. Open to suggestion

[accessibility-bot]

👋 Hello and thanks for pinging us! An accessibility first responder will review this soon.

• 💻 On PRs for our review: please provide a review environment with steps to validate, screenshots (with alt text), or videos demonstrating functionality we should be checking. This will help speed up our review and feedback cycle.
• ⚠️ If this is urgent, please visit us in #accessibility on Slack and tag the first responder(s) listed in the channel topic.

[khiga8]

Idk why the CI is failing when it passes locally

[kendallgassner]

I wish we could catch this for non primer components as well like any node_module components.

A future idea but also very complex and time consuming would be:

1. checking if the elementType is imported in the file your parsing

• if it is imported ->
  • find the file it was imported from resolve the file and look to see if the elementType is defined ->
    • if its defined check what the element_type of the returned component and repeat steps
    • if it is not defined check if it is imported and repeat previous step
• if it is not imported -> return element types

This ^ would be a complex but robust way to solve this problem.

• it would solve the issue for non primer components
• it would also solve the Link2 usecase:

const Link = ({children}) => {

return <a>{children}</a>
}


const Link2 = ({children}) => {

return <Link>{children}</Link>
}

[khiga8]

@kendallgassner this configuration could be set for non-Primer components too! for example, the prop name doesn't necessarily need to be as. It could be variant or something else.

[kendallgassner]

but you would need to set the settings so you would need to know every component you would be using?

EDIT: aka the team using this lint rule would need to potentially update the list anytime they introduced a new component?

NOTE: I think my idea is probably not worth the time -- its pretty complex 🤔 but wanted to document it

[khiga8]

@kendallgassner that's a very interesting idea. I think that could work if there's a 1:1 mapping between the component and HTML element type, but I don't think it would be able to handle if there's a prop that affects the HTML output, or logic in the code the component is being imported from that affect the outputted type. I'm also not totally sure if it's possible to deduce the type with static analysis (since the JS code within node_modules or wherever the component gets imported from won't actually be running when the linter runs).

[kendallgassner]

I know you can access code of imported files because of an eslint plugin I have worked on.... But I still think its a lot of work to do for this use case and I think unless we have more rules that would need it ..it probably doesn't make sense... I did like the href idea on the other pr you are working on-- I think it might mean that we wouldn't have to keep a settings section

I am not totally opposed to this method I just worry about maintainability

[khiga8]

I know you can access code of imported files

Do you know if we would be able to evaluate the code from an imported file within a linter context? I think this is the part I'm not really sure about.

For instance, say the linter comes across code like:

<SomeComponent as="button">

We are able to trace the component to a file which has a bunch of code including logic like this:

(pseudocode)

if (as==="button") {
  return (<button></button>)
} else if (as==="a") {
  return  (<a></a>)
} else {
  return  (<p></p>)
}

I think the linter can look at the shape of the code and try to predict the type, but I don't know if it can actually evaluate the code as we would in a runtime environment and determine the type accurately. Is this do-able? (I'm actually not sure. I know that with ERB linting, we can only make predictions based on the shape of the code and can't evaluate any Ruby code since it depends on the context to be set from the app running.)

[khiga8]

I am not totally opposed to this method I just worry about maintainability

Totally! I think we can reduce some of this burden by having a preset in eslint-plugin-primer-react that is updated so not every project has to define their own mappings with PRC components and can just use the preset. I don't think we need to map every component either but at least some simple components like <Link> and <Button>.

This particular rule I added targets links which can be identified with an href, but there are elements that won't have identifiable attributes like span or div which I can definitely forsee us adding rules for. This makes me think a settings config is inevitable. I'm also not sure we should confidently treat all components with href as a link, since it could simply be rendering a link child, but it itself is no a link.

I did try to align this component mapping approach with eslint-plugin-jsx-a11y. It seems like we'll ultimately need to maintain a component map for that plugin (unless they get rid of that feature) so I think that having a similar config for the rules we define wouldn't be too much more of a burden.

I appreciate your input! Let's ponder this a bit more! 🙏

[colebemis]

Love this 💖

I think we can reduce some of this burden by having a preset in eslint-plugin-primer-react that is updated so not every project has to define their own mappings with PRC components and can just use the preset.

👍

[kendallgassner]

Do you know if we would be able to evaluate the code from an imported file within a linter context? I think this is the part I'm not really sure about.

I should have linked my actual example in eslint-plugin-no-explicit-type-exports...

The example of parsing a file based off of an import

I think we can reduce some of this burden by having a preset in eslint-plugin-primer-react that is updated so not every project has to define their own mappings with PRC components and can just use the preset.

I also like this -- thank for clearing it up

[kendallgassner]

Also this is where the function I just linked is called in the linter

[khiga8]

Thanks all for your feedback! Let's go ahead with this approach for now and iterate on it as necessary. @kendallgassner and I discussed that we should revisit the AST parsing idea in the future since it is quite complex and requires more investigation.

Next up:

• I will introduce updates to README about this config
• open a PR in eslint-plugin-primer-react to create a preset.

I've checked out this branch in dotcom with the config and it's cool to see that it's already caught a few instances!