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

[Fix] jsx-no-literals: trim whitespace for allowedStrings check #2436

Merged
merged 1 commit into from Oct 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 24 additions & 6 deletions docs/rules/jsx-no-literals.md
@@ -1,11 +1,10 @@
# Prevent usage of string literals in JSX (react/jsx-no-literals)

There are a couple of scenarios where you want to avoid string literals in JSX. Either to enforce consistency and reducing strange behaviour, or for enforcing that literals aren't kept in JSX so they can be translated.
There are a few scenarios where you want to avoid string literals in JSX. You may want to enforce consistency, reduce syntax highlighting issues, or ensure that strings are part of a translation system.

## Rule Details

In JSX when using a literal string you can wrap it in a JSX container `{'TEXT'}`. This rules by default requires that you wrap all literal strings.
Prevents any odd artifacts of highlighters if your unwrapped string contains an enclosing character like `'` in contractions and enforces consistency.
By default this rule requires that you wrap all literal strings in a JSX container `{'TEXT'}`.

The following patterns are considered warnings:

Expand All @@ -19,14 +18,20 @@ The following patterns are **not** considered warnings:
var Hello = <div>{'test'}</div>;
```

### Options
```jsx
var Hello = <div>
{'test'}
</div>;
```

## Rule Options

There are two options:

* `noStrings` - Enforces no string literals used as children, wrapped or unwrapped.
* `allowedStrings` - an array of unique string values that would otherwise warn, but will be ignored.
* `allowedStrings` - An array of unique string values that would otherwise warn, but will be ignored.

To use, you can specify like the following:
To use, you can specify as follows:

```js
"react/jsx-no-literals": [<enabled>, {"noStrings": true, "allowedStrings": ["allowed"]}]
Expand All @@ -42,6 +47,12 @@ var Hello = <div>test</div>;
var Hello = <div>{'test'}</div>;
```

```jsx
var Hello = <div>
{'test'}
</div>;
```

The following are **not** considered warnings:

```jsx
Expand All @@ -59,6 +70,13 @@ var Hello = <div>{translate('my.translation.key')}</div>
var Hello = <div>allowed</div>
```

```jsx
// an allowed string surrounded by only whitespace
var Hello = <div>
allowed
</div>;
```

## When Not To Use It

If you do not want to enforce any style JSX literals, then you can disable this rule.
17 changes: 11 additions & 6 deletions lib/rules/jsx-no-literals.js
Expand Up @@ -40,10 +40,15 @@ module.exports = {
},

create(context) {
const isNoStrings = context.options[0] ? context.options[0].noStrings : false;
const allowedStrings = context.options[0] ? new Set(context.options[0].allowedStrings) : false;
function trimIfString(val) {
return typeof val === 'string' ? val.trim() : val;
}

const defaults = {noStrings: false, allowedStrings: []};
const config = Object.assign({}, defaults, context.options[0] || {});
config.allowedStrings = new Set(config.allowedStrings.map(trimIfString));

const message = isNoStrings ?
const message = config.noStrings ?
'Strings not allowed in JSX files' :
'Missing JSX expression container around literal string';

Expand All @@ -63,15 +68,15 @@ module.exports = {
}

function getValidation(node) {
if (allowedStrings && allowedStrings.has(node.value)) {
if (config.allowedStrings.has(trimIfString(node.value))) {
return false;
}
const parent = getParentIgnoringBinaryExpressions(node);
const standard = !/^[\s]+$/.test(node.value) &&
typeof node.value === 'string' &&
parent.type.indexOf('JSX') !== -1 &&
parent.type !== 'JSXAttribute';
if (isNoStrings) {
if (config.noStrings) {
return standard;
}
return standard && parent.type !== 'JSXExpressionContainer';
Expand All @@ -97,7 +102,7 @@ module.exports = {

TemplateLiteral(node) {
const parent = getParentIgnoringBinaryExpressions(node);
if (isNoStrings && parent.type === 'JSXExpressionContainer') {
if (config.noStrings && parent.type === 'JSXExpressionContainer') {
reportLiteralNode(node);
}
}
Expand Down
33 changes: 33 additions & 0 deletions tests/lib/rules/jsx-no-literals.js
Expand Up @@ -207,6 +207,15 @@ ruleTester.run('jsx-no-literals', rule, {
}
`,
options: [{allowedStrings: ['asdf']}]
}, {
code: `
class Comp1 extends Component {
render() {
return <div>asdf</div>
}
}
`,
options: [{noStrings: false, allowedStrings: ['asdf']}]
},
{
code: `
Expand All @@ -218,6 +227,20 @@ ruleTester.run('jsx-no-literals', rule, {
`,
options: [{noStrings: true, allowedStrings: ['&nbsp;']}]
},
{
code: `
class Comp1 extends Component {
render() {
return (
<div>
&nbsp;
</div>
);
}
}
`,
options: [{noStrings: true, allowedStrings: ['&nbsp;']}]
Copy link
Member

Choose a reason for hiding this comment

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

could we add a few more tests, one with leading and trailing spaces on allowedStrings, and a different one with them on the rendered text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apologies, i'm not sure how to action this request. could you say it another way?

for context, we happen to have a pre-existing test with spaces in allowedStrings that would fail if allowedStrings were not trimmed. here are all the combinations of spaces and not spaces with test coverage:

tested spaces in allowedStrings spaces in rendered text
🚫
🚫 🚫
🚫 🚫

is that the test gap you'd like me to fix?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly right :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Member

Choose a reason for hiding this comment

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

I'd also like a test with noStrings false, and an allowedStrings value :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ I've made noStrings: false explicit in the existing test case

},
{
code: `
class Comp1 extends Component {
Expand All @@ -227,6 +250,16 @@ ruleTester.run('jsx-no-literals', rule, {
}
`,
options: [{noStrings: true, allowedStrings: ['foo: ', '*']}]
},
{
code: `
class Comp1 extends Component {
render() {
return <div>foo</div>
}
}
`,
options: [{noStrings: true, allowedStrings: [' foo ']}]
}
],

Expand Down