-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[spike] Allow custom component to be mapped to element type #283
Conversation
When a default is set, all instances of the component will be mapped to the default. | ||
If a prop determines the type, it can be specified with `props`. | ||
|
||
For now, we only support the mapping of one prop type to an element type, rather than combinations of props. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -12,7 +12,7 @@ | |||
"scripts": { | |||
"pretest": "mkdir -p node_modules/ && ln -fs $(pwd) node_modules/", | |||
"eslint-check": "eslint-config-prettier .eslintrc.js", | |||
"test": "npm run eslint-check && eslint . && mocha tests/" | |||
"test": "npm run eslint-check && eslint . && mocha tests/**/*.js tests/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
👋 Hello and thanks for pinging us! An accessibility first responder will review this soon.
|
Idk why the CI is failing when it passes locally |
Co-authored-by: Ian Sanders <iansan5653@github.com>
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:
This ^ would be a complex but robust way to solve this problem.
const Link = ({children}) => {
return <a>{children}</a>
}
const Link2 = ({children}) => {
return <Link>{children}</Link>
} |
@kendallgassner this configuration could be set for non-Primer components too! for example, the prop name doesn't necessarily need to be |
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 |
@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 |
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 |
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:
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.) |
Totally! I think we can reduce some of this burden by having a preset in This particular rule I added targets links which can be identified with an 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! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
👍
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 also like this -- thank for clearing it up |
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've checked out this branch in dotcom with the config and it's cool to see that it's already caught a few instances! |
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.
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.This above setting specifies no default. It instead says that if there's a prop type of
as
and if that prop isundefined
OR is defined and set toa
, the component should be mapped to ana
tag. This ensures that ifas
is set to some other value that isn't configured in props likesummary
, we keep the raw type ofLink
(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.