jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
9.01k stars 2.77k forks source link

[New] `jsx-no-literals`: add `elementOverrides` option #3812

Closed Pearce-Ropion closed 2 months ago

Pearce-Ropion commented 2 months ago

Resolves #3808

Preface - I didn't intend to rewrite the whole rule, but I ended up needing to re-write the way the rule fetches and applies the config which resulted in the majority of the rule being reworked.

What is the purpose of this PR?

Adds a new option, elementOverrides which adds the ability to override the rule options for any named component. Specifying options within elementOverrides creates a new context for the rule, so the rule will only apply the new options to the specified element and its children (if applyToNestedElements is true - see below). This means that the root rule options will not apply to the specified element.

elementOverrides is an object where the keys are the element names and the values are objects with the same options that this rule previously supported (noStrings, allowedStrings, noAttributeStrings and ignoreProps). elementOverrides also supports 2 new options: allowElement and applyToNestedElements.

The element name must match the following regular expression. It does not support intrinsic JSX elements (ie. div or span) and all elements that do not match the pattern will be filtered out when the config is normalized. The element name can also be a compound/namespaced/sub component (JSXMemberExpression) like Modal.Button or React.Fragment. If an element name like Fragment is specified, the override config will match on either of the following components:

<Fragment>text</Fragment>
<React.Fragment>text</React.Fragment>

The rule will also construct a simple import (or require) map specifically for renamed imports (ie import { A as B } from 'c';. The rule will match on the original (imported) specifier instead of the renamed (local) specifier.

What should reviewers focus on?

Most of the actual rule/option logic has remained unchanged. However, I re-ordered some of the node visitors for performance with regards to when getOverrideConfig is called since it is a relatively expensive function. Also, a lot of the visitor functions needed the current config passed in which required some changes within those too.

How is this PR tested?

This PR adds roughly 50 tests. Each option (new and old) is individually tested for both valid and invalid cases. For each option, there are roughly 3 states that are tested:

  1. Just an override
  2. An override with nested elements
  3. A root config and an override

All extraneous features are also tested such as imports/requires renames, the element name pattern and compound components

Based on a manual scan of the coverage report, I think all branches that need to be covered have been covered by tests. There are a few extra that exist to validate type check.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.79%. Comparing base (a2306e7) to head (e8f20fd).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3812 +/- ## ========================================== + Coverage 97.49% 97.79% +0.29% ========================================== Files 132 135 +3 Lines 9707 9825 +118 Branches 3554 3609 +55 ========================================== + Hits 9464 9608 +144 + Misses 243 217 -26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Pearce-Ropion commented 2 months ago

Not sure what to do about the failing codecov/project check. I checked codecov while I was testing and I believe I have covered all branches, statements and functions.

ljharb commented 2 months ago

Don’t worry about it :-)