Skip to content
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

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

Merged

Conversation

Pearce-Ropion
Copy link
Contributor

@Pearce-Ropion Pearce-Ropion commented Sep 1, 2024

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.

  • allowElement (default: false) - When true the rule will allow the specified element to have string literals as children, wrapped or unwrapped without warning. If the noStrings option is also specified, then only unwrapped literals will be allowed through. This option satisfies [Feature Request]: Add allowedElements option to jsx-no-literals #3808.
  • applyToNestedElements (default: true) - When false the rule will not apply the current options set to nested elements. This is useful when you want to apply the rule to a specific element, but not to its children.

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.

Copy link

codecov bot commented Sep 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review.

lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Outdated Show resolved Hide resolved
lib/rules/jsx-no-literals.js Show resolved Hide resolved
@Pearce-Ropion Pearce-Ropion force-pushed the pearce/no-literals-allowed-elements branch from d676a54 to 3c98eb2 Compare September 3, 2024 02:22
@ljharb ljharb force-pushed the pearce/no-literals-allowed-elements branch from 2608777 to 8fa7280 Compare September 10, 2024 23:29
docs/rules/jsx-no-literals.md Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the pearce/no-literals-allowed-elements branch from 8fa7280 to b8217ed Compare September 10, 2024 23:31
@Pearce-Ropion
Copy link
Contributor Author

Pearce-Ropion commented Sep 11, 2024

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
Copy link
Member

ljharb commented Sep 11, 2024

Don’t worry about it :-)

@ljharb ljharb merged commit b8217ed into jsx-eslint:master Sep 11, 2024
341 of 342 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add allowedElements option to jsx-no-literals
2 participants