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

Allow for stricter no-literals rule #1202

Merged
merged 8 commits into from
Jul 2, 2017
Merged

Conversation

deecewan
Copy link
Contributor

@deecewan deecewan commented May 17, 2017

Hey,

I was going to make a separate plugin to do this, but I feel like it can fit quite nicely here.

I've added an option to disallow all string literals in components. We translate all text inside our components, so don't want any strings in the JSX, as they then cannot be translated.

Strings are still allowed inside of function calls inside of JSXExpressionContainers so that translation systems requiring a string key can still be used.

*/
'use strict';

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------

var NO_STRINGS = 'no-strings';
Copy link
Member

Choose a reason for hiding this comment

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

const

function getValidation(node) {
const standard = !/^[\s]+$/.test(node.value) &&
node.parent &&
node.parent.type.indexOf('JSX') !== -1 &&
Copy link
Member

Choose a reason for hiding this comment

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

Will node.parent always have a type property that's a string or an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the existing validation. I just removed part of it if you enable the option.

{'Test'}
</Foo>
`,
parser: 'babel-eslint',
Copy link
Member

Choose a reason for hiding this comment

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

Please also add identical test cases that use the default parser.

@@ -17,6 +20,8 @@ module.exports = {
},

schema: [{
enum: [NO_STRINGS]
Copy link
Member

Choose a reason for hiding this comment

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

Rather than add a string option, let's add this to the options object, for future extensibility.


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

Choose a reason for hiding this comment

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

ftr tho, this shouldn't ever happen, since straight quotes are typographically incorrect - and this would be a bug in the highlighter. I'm not sure this is worth mentioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the original text, essentially, with some modifications because of the different structure. If what you're saying is true, then this rule has/had no purpose.

Copy link
Member

Choose a reason for hiding this comment

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

It still has many purposes; but dealing with quotes isn't one of them imo :-)

Fine to leave the text as-is.

To use, you can specify like the following:

```json
"react/jsx-no-literals": [{"noStrings": false}]
Copy link
Member

Choose a reason for hiding this comment

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

would this not be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhhhh, because i had a mind blank.

@deecewan
Copy link
Contributor Author

thanks for the feedback. what else would you like for this one?

@yannickcr yannickcr merged commit 31e55d2 into jsx-eslint:master Jul 2, 2017
@yannickcr
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants