jsx-eslint / eslint-plugin-react

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

[Feature Request]: Add `allowedElements` option to `jsx-no-literals` #3808

Closed Pearce-Ropion closed 3 days ago

Pearce-Ropion commented 2 weeks ago

What rule do you want to change?

jsx-no-literals

How do you think the change should be implemented?

I would like to see an option to add specific JSXElements to an allow list in jsx-no-literals. These elements would be allowed to have literals as children. I am unsure if this should also apply the other options that this rule provides such as allowing literals in props.

The new option could be named allowedElements and accept an array of allowed JSX intrinsic element names as well as named JSX element. If the named element is imported, it should use the imported name (so Button not MyButton in the case of import { Button as MyButton } from './Button';. If the import is a default import, it should match against the name given in the import (so MyButton in the case of import MyButton from './Button';.

I am unsure of the best way to handle namespaced elements as I could see it going either way. Maybe check each portion of the namespaced name as well as the whole thing.

Example

/* eslint react/jsx-no-literals: ["error", { "allowedElements": ["div"] }] */

<span>This should error</span>
<div>This should pass</div>

Additional Info

Currently, we use jsx-no-literals to prevent JSXText from appearing as the child of an element and instead require developers to pass their copy in via a translation function. This is similar to one of the examples in the jsx-no-literals docs. However, we use react-i18next which has a <Trans> component which will automatically pull all JSXText as valid translatable copy. We don't encourage our devs to use this method as it currently violates jsx-no-literals but would like to make this method less "hacky" when it is needed.

/* eslint react/jsx-no-literals: ["error", { "allowedElements": ["Trans"] }] */

<div>This should error</div>
<Trans>This should <a href="#">pass</a></Trans>

Participation

ljharb commented 2 weeks ago

ah ok, so it's not about div/span, but about custom components specifically? That makes sense to me - at Airbnb we had a <T> component for the same use case.

Pearce-Ropion commented 2 weeks ago

ah ok, so it's not about div/span, but about custom components specifically? That makes sense to me - at Airbnb we had a <T> component for the same use case.

Right, although I think it should still accept an intrinsic element if a user would like it to be configured that way.

How would you expect this option to interact with the other options this rule supports? Do you think it should basically constitute as a blanket "ignore anything with this element name", thus render all of the other options that affect the given element inert?

Or, should there be multiple versions of this option to allow enabling/disabling it for the other options? For example, the type could look something like this:

allowedElements?:
  | string[];
  | {
      elements: string[];
      noStrings?: boolean;
      allowedStrings?: boolean;
      ignoreProps?: boolean;
      noAttributeStrings?: boolean;
    };

Or you might be able to specify the list per option like this:

allowedElements?:
  | string[];
  | {
      noStrings?: string[];
      allowedStrings?: string[];
      ignoreProps?: string[];
      noAttributeStrings?: string[];
    }
ljharb commented 2 weeks ago

I'd prefer not to allow html elements to start - if someone wants that, they'd need to provide a compelling justification.

hm, good question. what about a config option elementOverrides, that takes an object, whose keys are a custom element name (if it doesn't start with a capital letter it's invalid), and the value is the root config object minus elementOverrides?

Pearce-Ropion commented 2 weeks ago

Ooh, I like it!