Tools: ☂️ [PAUSED] React lint rules

[sebmck]

This issue serves to discuss what React rules we should have in Rome. Ideally these rules would be blessed by the React team.

What rules do you think are the most important? Keep in mind that Rome does not allow disabling lint rules. You can only suppress them. The React rules will not be an exception. Rome allows AST autofixes which drastically simplifies the existence of many lint rules by allowing them to be automatically fixed, and with the Rome review system, unsafe suggestions are also easily added and amended.

[sebmck]

It would be nice to have a strong set of accessibility rules. We could have autofixes/suggestions that infer from usage what props the author probably meant.

[Daniel15]

FWIW here's the list of lint rules included with Create React App, which is probably a good guide as to which rules a lot of people are using today: https://github.com/facebook/create-react-app/blob/master/packages/eslint-config-react-app/index.js

The "Rules of Hooks" rule is a pretty important one for React hooks: https://github.com/facebook/react/tree/master/packages/eslint-plugin-react-hooks

It would be nice to have a strong set of accessibility rules.

Create React App uses eslint-plugin-jsx-a11y by @evcohen and @jessebeach which has some decent rules: https://github.com/evcohen/eslint-plugin-jsx-a11y/

[jamiebuilds]

Spreadsheet of all rules with related info and statuses

[sebmck]

A lot of those seems so superfluous. We can pretty much ignore any of the rule that are specific to createClass. Also there’s many there that just get around ESLints poor handling of JSX, that we already handle such as variables in JSX being marked as used.

[ljharb]

There’s also the Airbnb config’s react rules worth considering.

[diokey]

Are react specific rules going to be implemented as expansion packs as discussed in https://github.com/facebookexperimental/rome/issues/173 ? It didn't seem like the discussion was conclusive. Maybe this is a good time to make that decision.

[sebmck]

Thinking about expansion packs, it feels like having an on switch just for the sake of it. We can detect the React code pretty unambiguously so there's no reason to have configuration to turn it on.

[tatchi]

Something that we might consider: https://twitter.com/dan_abramov/status/1255237989548134405?s=20

[macovedj]

Is it safe to assume that the recommended rules are not superfluous? If there's not going to be an expansion pack, could people have at those ones?

[VictorHom]

If no one has started working on jsx-a11y/accessible-emoji, can I work on it?

[VictorHom]

I'm going to take a look at implementing: react/no-did-update-set-state if no one has started 🙏

[sebmck]

Before submitting React or JSX lint rules of any kind please get confirmation in this issue or on the Discord.

[sebmck]

@jamiebuilds Could you look at finalizing the rule list? The comment at the top with a link in big bold letters looks like it's finalized and ready for people to work on. While a lot of them are fine, I don't want someone to spend a lot of time on ones that we don't want only to have their PR rejected.

[bitpshr]

Grabbing jsx-a11y/no-autofocus, jsx-a11y/no-access-key, and jsx-a11y/scope.

[bitpshr]

Grabbing jsx-a11y/autocomplete-valid, jsx-a11y/heading-has-content, and jsx-a11y/no-distracting-elements.

[VictorHom]

grabbing:
react/no-will-update-set-state,
aria-unsupported-elements
anchor-has-content

[ematipico]

I would like to work on jsx-a11y/lang

[VictorHom]

going to take a look at:
no-autofocus
no-distracting-elements
no-access-key
media-has-caption

[bitpshr]

@VictorHom just a heads up, jsx-a11y/no-autofocus, jsx-a11y/no-distracting-elements, and jsx-a11y/no-access-key should already be done.

[ematipico]

I'd like to tackle:

• jsx-a11y/mouse-events-have-key-events
• jsx-a11y/role-has-required-aria-props
• jsx-a11y/tabindex-no-positive

[Kelbie]

I'll have a go at jsx-a11y/aria-props

[VictorHom]

If possible, I want to take a look at react-hooks/rules-of-hooks unless someone else is working on it

[VictorHom]

Also, I would like to take a look at jsx-no-useless-fragment

[bitpshr]

Has the spreadsheet above been fully triaged? Is the idea to implement all rules listed in that document, except for those marked in red as explicitly unnecessary? I'm personally in favor of a very opinionated set of lint rules as long as they can be silenced.

cc @sebmck @jamiebuilds

[sebmck]

@bitpshr

Grabbing import/no-anonymous-default-export.

We have the rule defaultExportSameBasename that enforces that the default export id of a module matches. Right now it just checks id !== undefined so we should modify that rule. We can also rename it since the name is weird.

Has the spreadsheet above been fully triaged? Is the idea to implement all rules listed in that document, except for those marked in red as explicitly unnecessary? I'm personally in favor of a very opinionated set of lint rules as long as they can be silenced.

I think that document is up to date? I know @jamiebuilds was trying to find the time to finalize it. The ones in red we definitely don't want, same with general createClass rules. Class components are still fair game though. I'm also in favor of very opinionated rules, even if you need to suppress it in a limited set of circumstances.

We can also have rules not in that list. Maybe there are some rules that are easier to write in Rome that aren't in eslint, or that are easier to justify with Rome's better autofixes.

[jamiebuilds]

So here's a list of things we generally want to avoid:

• Rules specific to APIs that have been deprecated for awhile React.createClass/etc
• Rules where they are not useful (or not useful enough) without some additional configuration (Example: react/forbid-component-props)
• Rules for things that are/can be handled by the formatter in Rome (Example: react/jsx-curly-newline)
• Rules that enforce "silly" syntactic things, like sorting object properties, or forcing you to use one syntax over another without any concrete benefits (Example: react/function-component-definition)
• Rules that try to enforce something that they can't really enforce without things like a type-information or side-effect tracking. (Example: react/no-access-state-in-setstate doesn't enforce the thing its trying to enforce)
• Rules that aren't necessary in Rome (Example: react/jsx-uses-react)

If you have a question about a particular lint rule, please reach out and ask about it before you go and implement it

[ljharb]

Agree with that list, except that function-component-definition does have concrete benefits (whether you care about them or not) :-)

[bitpshr]

Currently working on:

• react/button-has-type
• react/no-did-mount-set-state
• react/no-redundant-should-component-update
• react/no-unsafe

[smlmrkhlms]

I will have an initial pass at jsx-a11y/no-onchange if it is unclaimed.

[VictorHom]

I am going to take on jsx-fragments since I am looking at jsx-no-useless-fragment as well if that's alright.

A question I have is, in jsx-fragments do we want to enforce one <>...</> over <React.Fragment>...<React.Fragment> or <Fragment/>...</Fragment>?

@romejs/contributors

[sebmck]

@VictorHom I think we do. I can't imagine why you'd want React.Fragment instead of <>.

[jamiebuilds]

Does <> have a key syntax yet?

[ljharb]

That is indeed the primary reason why you must use React.Fragment; the other is if an API takes a "component", and the component you want to pass is React.Fragment.

[VictorHom]

Does it always have to be <React.Fragment>?

Would we maybe want to only support <Fragment> and <>? (I ask because of how the ast looks using , but wanted to clarify)

I'll still want to prefer <> over <Fragment> unless key is being used ( for rule jsx-fragments)

And then for jsx-no-useless-fragment, I want to check if supporting <Fragment> and <> is enough (but that is dependent on what we think about supporting <React.Fragment>, <Fragment>, and <> or just <Fragment>, and <>

[bitpshr]

I think that we should support both <Fragment> and <React.Fragment> for cases where a key is used. I feel that it's overly-opinionated to mandate destructured imports. For what it's worth, most other rules that check for members on the default React export check for both cases, e.g. React.createElement and createElement. If they don't, we'll probably have to do a pass to make sure that such cases are uniformly handled throughout all rules.

[smlmrkhlms]

Would it be okay if I start working on react/no-string-refs?

[sebmck]

Go for it!

[smlmrkhlms]

I can start working on react/no-is-mounted if there are no objections.

[bitpshr]

@smlmrkhlms: I don't think we need to implement react/no-is-mounted since we're ignoring rules and logic paths within rules that only apply to createReactClass. Apologies, I'll try to get the document updated.

[VictorHom]

gonna take a look at click-events-have-key-events if no one is working on it

will follow up on jsx-no-useless-fragment after I address jsx-fragments (not that they are dependent)

[VictorHom]

going to also take a look at anchor-is-valid rule if no one is working on it.

[ematipico]

Working on:

• jsx-a11y/no-noninteractive-tabindex
• jsx-a11y/no-noninteractive-element-to-interactive-role
• jsx-a11y/no-redundant-roles

[VictorHom]

I'll look at no-access-state-in-setstate if no one is working on that

[VictorHom]

I'll take a look at react/no-array-index-key if no one is working on it

[VictorHom]

I'll take a look at jsx-pascal-case if no one is working on it.

[yeonjuan]

I want to work on react/jsx-no-literals. :) - this rule is not accepted in this project. #560

[VictorHom]

I would like to take a look at react-hooks/rules-of-hooks if no one is looking at that one

Also this one no-this-in-sfc if possible

[UsamaHameed]

I would like to take a look at react/default-props-match-prop-types if no one else is already working on it.

[VictorHom]

There's been other things happening recently, but I am still working on these rules:
no-this-in-sfc
react-hooks/rules-of-hooks
react/no-array-index-key

[yeonjuan]

Can I take a react/require-render-return? - #578

[Kelbie]

I'll take autocomplete-valid

[yeonjuan]

I'll take react/no-render-return-value.

[VictorHom]

gonna start on react-hooks/rules-of-hooks

I'll also take a look at react/jsx-props-no-spreading

[stefanuros]

Ill take a look at react/sort-comp

[VictorHom]

Still working on react-hooks/rules-of-hooks. I should have something soon

[iaravindreddyp]

can i try react/state-in-constructor ?

[sebmck]

Hey everyone! I'd like to pause development of new React lint rules (if you're currently working on or have reserved something in this thread then feel free to complete it!). We're preparing for our first release and should focus on stability and polish.

We need to take a more comprehensive look on the sort of React rules and patterns we want to encourage (ideally with approval from the React team or "community leaders" (whatever that means lol)). For that reason there may be some rules that will undergo significant tweaks or even removal. I'll make sure to keep this thread updated with those discussions and as we finalize any guidelines in case anyone wants to get involved.

Thank you everyone for your participation so far! Really exciting to see the contributions.

[sebmck]

Going to close this and use more finegrained issues.