Skip to content

[new] [jsx-curly-brace-presence] Disallow unnecessary JSX expressions or enforce them #1349

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 37 commits into from
Aug 30, 2017
Merged

[new] [jsx-curly-brace-presence] Disallow unnecessary JSX expressions or enforce them #1349

merged 37 commits into from
Aug 30, 2017

Conversation

jackyho112
Copy link
Contributor

@jackyho112 jackyho112 commented Aug 7, 2017 •
edited
Loading

For issue #1310:

This rule checks for unnecessary JSX expressions when literals alone will do or enforce the presence of JSX expressions.

References:
String literals in JSX
JSX gotchas

@jackyho112 jackyho112 closed this Aug 7, 2017
@ljharb ljharb mentioned this pull request Aug 7, 2017
Closed
@jackyho112 jackyho112 reopened this Aug 7, 2017
@jackyho112 jackyho112 closed this Aug 7, 2017
@jackyho112 jackyho112 deleted the add-new-rule-no-unnecessary-curly-brace-squashed branch August 7, 2017 23:48
@jackyho112 jackyho112 restored the add-new-rule-no-unnecessary-curly-brace-squashed branch August 7, 2017 23:49
@jackyho112 jackyho112 reopened this Aug 7, 2017
@jackyho112 jackyho112 changed the title Add optional rule to disallow unnecessary JSX expressions Add optional rule to disallow unnecessary JSX expressions or enforce them Aug 8, 2017
ljharb
ljharb requested changes Aug 8, 2017
View reviewed changes
lib/rules/jsx-curly-brace-presence.js Outdated
},
additionalProperties: false
}
props: {enum: optionValues},
Copy link
Member

Choose a reason for hiding this comment

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

this should probably still specify the default?

lib/rules/jsx-curly-brace-presence.js Outdated
additionalProperties: false
}
props: {enum: optionValues},
children: {enum: optionValues}
Copy link
Member

Choose a reason for hiding this comment

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

also here?

lib/rules/jsx-curly-brace-presence.js Outdated
@@ -108,7 +109,7 @@ module.exports = {
// --------------------------------------------------------------------------

return {
JSXExpressionContainer: node => {
JSXExpressionContainer: function(node) {
Copy link
Member

Choose a reason for hiding this comment

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

any particular reason to change these from arrows?

lib/rules/jsx-curly-brace-presence.js Outdated
@@ -119,7 +120,7 @@ module.exports = {
}
},

Literal: node => {
Literal: function(node) {
Copy link
Member

Choose a reason for hiding this comment

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

also here?

@jackyho112 jackyho112 force-pushed the add-new-rule-no-unnecessary-curly-brace-squashed branch from 03e5fcf to 2d695c4 Compare August 8, 2017 05:01
jackyho112
jackyho112 commented Aug 8, 2017
View reviewed changes
lib/rules/jsx-curly-brace-presence.js Outdated
@@ -27,8 +27,8 @@ module.exports = {
{
type: 'object',
properties: {
props: {enum: optionValues},
children: {enum: optionValues}
props: {enum: optionValues, default: optionNever},
Copy link
Contributor Author

@jackyho112 jackyho112 Aug 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the default by mistake. They are back now.

jackyho112
jackyho112 commented Aug 8, 2017
View reviewed changes
lib/rules/jsx-curly-brace-presence.js Outdated
@@ -116,15 +116,15 @@ module.exports = {
// --------------------------------------------------------------------------

return {
JSXExpressionContainer: function(node) {
JSXExpressionContainer: node => {
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 made the change to maintain consistencies across files, but I guess it doesn't matter anyway. I have changed it back to using the arrow function.

@jackyho112
Copy link
Contributor Author

@ljharb
Well, I hope my attempt wasn't too embarrassing. I am not sure why the travis build failed. All the tests passed for me. Let me know if you have any feedback to improve my PR.

ljharb
ljharb requested changes Aug 8, 2017
View reviewed changes
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.

It might be useful to also allow, in addition to the object, a single string of "always" or "never" that sets the defaults to that value.

Can you add tests that include spread props, as well as tests like

<div a={`foo`} />
<div a={`foo${bar}`} />

?

@ljharb ljharb added the new rule label Aug 8, 2017
@jackyho112
Copy link
Contributor Author

@ljharb Good suggestions! I will make the changes.

@jackyho112
Copy link
Contributor Author

jackyho112 commented Aug 9, 2017 •
edited
Loading

@ljharb
I added tests for template literal and spread props. I also accounted for template literal in the rule and added a new rule option to allow passing in a single string to change the default. Let me know if there is any more change you have in mind. Thanks!

ljharb
ljharb requested changes Aug 16, 2017
View reviewed changes
docs/rules/jsx-curly-brace-presence.md Outdated
They are `always`, `always,single`, `always,double`, `always,orignal`, `never` and `ignore` for checking on JSX props and children.

* `always`: always enforce curly braces inside JSX props or/and children and fix with double quotes inside the generated JSX expressions
* `always,single`: always enforce curly braces and fix with single quotes
Copy link
Member

Choose a reason for hiding this comment

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

so, first - i think rather than squashing the quote style into the string value, this should be an object.

However, quote style is something that another rule should handle. This one should just pick a default - i'd go with double quotes - and let the jsx-quotes rule handle correcting it.

ljharb
ljharb reviewed Aug 16, 2017
View reviewed changes
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.

Thanks, this is looking much better.

docs/rules/jsx-curly-brace-presence.md Outdated

```js
...
"react/forbid-elements": [<enabled>, { "props": <string>, "children": <string> }]
Copy link
Member

Choose a reason for hiding this comment

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

this might be the wrong rule name

docs/rules/jsx-curly-brace-presence.md Outdated

```js
...
"react/forbid-elements": [<enabled>, <string>]
Copy link
Member

Choose a reason for hiding this comment

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

also here

docs/rules/jsx-curly-brace-presence.md Outdated
It can fixed to:

```jsx
<App prop='foo'>Hello world</App>;
Copy link
Member

Choose a reason for hiding this comment

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

let's use multiple props in this example so we can show that it allows either single or double quotes (one of each)

@jackyho112
Copy link
Contributor Author

@ljharb

Thanks for all your suggestions! They all make sense and give me an idea on how to better contribute to this repo for future PRs. Hopefully, everything looks good now!

ljharb
ljharb reviewed Aug 21, 2017
View reviewed changes
docs/rules/jsx-curly-brace-presence.md Outdated

```jsx
<App>Hello world</App>;
<App prop='Hello world' attr="foo" />;
Copy link
Member

Choose a reason for hiding this comment

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

should this example use double quotes consistently?

docs/rules/jsx-curly-brace-presence.md Outdated

```jsx
<App>{"Hello world"}</App>;
<App prop={'Hello world'}>{'Hello world'}</App>;
Copy link
Member

Choose a reason for hiding this comment

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

should both of these examples use the same quote style?

docs/rules/jsx-curly-brace-presence.md Outdated
They can be fixed to:
```jsx
<App>{"Hello world"}</App>;
<App prop={'Hello world'} attr={"foo"}>{"Hello world"}</App>;
Copy link
Member

Choose a reason for hiding this comment

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

these should probably both use the same kind of quotes

docs/rules/jsx-curly-brace-presence.md Outdated
It can fixed to:

```jsx
<App prop='foo' attr="bar">Hello world</App>;
Copy link
Member

Choose a reason for hiding this comment

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

these should probably both use the same kind of quotes

docs/rules/jsx-curly-brace-presence.md Outdated

## Edge cases

The fix also deals with template literals, strings with quotes and strings with escapes characters.
Copy link
Member

Choose a reason for hiding this comment

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

please use the Oxford comma - in this case, after "quotes"

docs/rules/jsx-curly-brace-presence.md Outdated
will warned and fixed to:

```jsx
<App prop={'Hello "foo" world'}>{"Hello 'foo' \"bar\" world"}</App>;
Copy link
Member

Choose a reason for hiding this comment

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

is it expected that the quote style is unchanged for prop?

@jackyho112
Copy link
Contributor Author

jackyho112 commented Aug 22, 2017 •
edited
Loading

@ljharb

Thanks again for all your suggestions. I was leaving original quotes in as much as I could previously when I was fixing the styles, but your suggestion makes sense for the purpose of consistency. I have changed the operations to fix with double quotes in all cases. Hope things look better now!

@jackyho112 jackyho112 changed the title Add optional rule to disallow unnecessary JSX expressions or enforce them [jsx-curly-brace-presence] Disallow unnecessary JSX expressions or enforce them Aug 23, 2017
@jackyho112 jackyho112 changed the title [jsx-curly-brace-presence] Disallow unnecessary JSX expressions or enforce them [new] [jsx-curly-brace-presence] Disallow unnecessary JSX expressions or enforce them Aug 23, 2017
ljharb
ljharb approved these changes Aug 29, 2017
View reviewed changes
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.

Thanks, this looks great!

@jackyho112
Copy link
Contributor Author

@ljharb Thank you for the review!

@ljharb ljharb merged commit 07b348d into jsx-eslint:master Aug 30, 2017
@jackyho112 jackyho112 deleted the add-new-rule-no-unnecessary-curly-brace-squashed branch June 28, 2018 23:05
@ljharb ljharb mentioned this pull request Apr 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jseminck jseminck jseminck left review comments

@yannickcr yannickcr yannickcr approved these changes

@ljharb ljharb ljharb approved these changes

@lencioni lencioni Awaiting requested review from lencioni

@EvHaus EvHaus Awaiting requested review from EvHaus

Assignees
No one assigned
Labels
new rule
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jackyho112 @yannickcr @ljharb @jseminck