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

Is there any way to multiline all JSX props if there are more than X props on element? #2735

Closed
Onatolich opened this issue Jul 30, 2020 · 25 comments

Comments

@Onatolich
Copy link

Hello, I want to achieve a behavior when JSX props will all be multiline if amount of props is more than defined value. Currently there is react/jsx-max-props-per-line that is the closest to what I need, but if I set its maximum to 2, it groups props in multiple lines grouped by 2. So this JSX:

<Component prop1="value 1" prop2="value 2" prop3="value 3" prop4="value 4" />

Will be formatted to this:

<Component
  prop1="value 1" prop2="value 2"
  prop3="value 3" prop4="value 4"
/>

While I want to achieve this:

<Component
  prop1="value 1"
  prop2="value 2"
  prop3="value 3"
  prop4="value 4"
/>

Is it possible to add a rule like react/jsx-props-multiline with attribute like after with amount of props after which it should be multiline?

@ljharb
Copy link
Member

ljharb commented Jul 31, 2020

I think a reasonable thing to add to jsx-max-props-per-line would be to allow maximum to be, in addition to a number x like it is now, an object like { "single": y, "multi": x }. That way, you could configure the rule like this:

"react/jsx-props-multiline": ["error", {
  "maximum": {
    "single": 2,
    "multi": 1
  },
  "when": "always"
}],

which would, for single-line tags, error on 3+ props, and for multiline tags, error on 2+ props.

This object form of "maximum" would only make sense when when is set to "always" or omitted.

Thoughts?

@jzabala
Copy link
Contributor

jzabala commented Aug 2, 2020

@ljharb

I think this is a duplicate of #2598.

Regarding the implementation how about if instead of an object we defined two new properties:

"react/jsx-props-multiline": ["error", {
  "maxSingular": 2,
  "maxMulti": 1,
  "when": "always"
}],

We deprecate maximum, and to preserve the current behavior if not set, maxSingular and maxMulti will be equal to maximum.

@ljharb
Copy link
Member

ljharb commented Aug 2, 2020

What’s the advantage to having the object be flat?

@jzabala
Copy link
Contributor

jzabala commented Aug 2, 2020

What’s the advantage to having the object be flat?

We would avoid a breaking change in the next version and it would be easier to validate.

We won't have to evaluate for the maximum too. Something like:

const maximum = (config && config.maximum) || {}`.
const singular = maximum.singular || defaultSingular
const multi = maximum.multi || defaultMulti

If we already have the config object, it would be just:

const singular = config.maxSingular  || defaultSingular
const multi = config.maxMulti || defaultMulti

@ljharb
Copy link
Member

ljharb commented Aug 2, 2020

What would be a breaking change? We'd support both a number, or an object, under maximum, in my proposal.

I'm more concerned with the cleanliness of the config than with using 2 vs 3 lines in the rule :-)

@jzabala
Copy link
Contributor

jzabala commented Aug 2, 2020

What would be a breaking change? We'd support both a number, or an object, under maximum, in my proposal.

Oh sorry. Did not get the part that both would be supported. Awesome. In favor of object too 😁

@iiison
Copy link
Contributor

iiison commented Aug 15, 2020

is anybody working on this? If not, I can give this a shot...

@jzabala
Copy link
Contributor

jzabala commented Aug 15, 2020

is anybody working on this? If not, I can give this a shot...

Go for it 👍🏼

@iiison
Copy link
Contributor

iiison commented Aug 17, 2020

@ljharb | @jzabala does it make sense to change the property name single and multi to singleLine and multiLine respectively? It would be more intuitive.

@ljharb
Copy link
Member

ljharb commented Aug 17, 2020

@iiison what would "single" or "multi" refer to that wasn't pretty obviously "lines"?

@iiison
Copy link
Contributor

iiison commented Aug 17, 2020

well, it is. It didn't make sense to me in the first look. Maybe it's just me. I will keep single and multi 😀

@iiison
Copy link
Contributor

iiison commented Aug 17, 2020

@ljharb I have a small confusion(it may sound obvious), say this is the config for the rule:

"react/jsx-props-multiline": ["error", {
  "maximum": {
    "single": 2,
    "multi": 1
  },
  "when": "always"
}]

And this is user's code:

<App foo bar baz demo val /> 

As this is a single line tag, the rule will throw an error by asking to put 2 props per line to make it look like:

<App 
  foo bar 
  baz demo 
   val 
/> 

But now, if you run the linter again, it with throw the error again to put 1 prop per row, as it's a multi-line tag now. I see 2 ways to tackle this situation, a) To check the code for multi-line tag rule once the single line tag rule is completed. b) To make the user provide only one rule, either single or multi.

Please let me know if this makes sense.

@ljharb
Copy link
Member

ljharb commented Aug 17, 2020

I believe eslint already does multiple passes on the command line in order to ensure that no further autofixes are needed.

However, it would be ideal for the rule to suggest the correct fix in the first place (ie, your option "a" there).

@iiison
Copy link
Contributor

iiison commented Aug 18, 2020

Okay, few things about the option "a":

  • That means single will most likely be overruled by multi. Which, I feel makes single a little redundant(maybe it's my personal opinion).
  • This one is a little tricky, suppose the user has this config:
"react/jsx-props-multiline": ["error", {
  "maximum": {
    "single": 1,
    "multi": 2
  },
  "when": "always"
}]

And this the code:

<App foo bar  /> 

In this case, single would ask the user to make one prop each line. But as multi gets verified after single, the actual error(when the fix wasn't triggered) in the console will ask to put two props in a line, and that is already there in the code.

Though I completely understand these configs don't make much sense, but it's a possible option. One solution could be to mandate the user to keep single > multi.

Let me know what you think 😊

@jzabala
Copy link
Contributor

jzabala commented Aug 18, 2020

In that case @iiison the rule would ask to put the props in one line but not in the initial line. With the config you are describing and the example code the valid version would be:

<App
  foo bar
/> 

@iiison
Copy link
Contributor

iiison commented Aug 19, 2020

Sounds like a good idea to me! Let me start the implementation then 😊

@ljharb
Copy link
Member

ljharb commented Aug 19, 2020

I think that there is certainly a possibility of conflict if some folks want single > multi, and others want multi > single.

I think we can implement with this plan, and if folks want to flip the priority, we can add another config option (that defaults to single > multi) that can be set to multi > single.

@jzabala
Copy link
Contributor

jzabala commented Aug 19, 2020

I think that there is certainly a possibility of conflict if some folks want single > multi, and others want multi > single.

I think we can implement with this plan, and if folks want to flip the priority, we can add another config option (that defaults to single > multi) that can be set to multi > single.

If we evaluate based on the start and ending line of the component we might avoid any conflicts.

For example, for single line declarations:

<App foo bar  />

there is no need to run the multiline code. It will ask if the number of props is greater than single and if that is the case suggest the corresponding multiline fix.

And for multiline declarations:

<App 
  foo fooz
  bar baz
/>

there is no need to run the single line code. It will ask if the number of props per line is greater than multi and if that is the case suggest the corresponding multi or single fix.

That way we can avoid adding another config option to the rule.

@iiison
Copy link
Contributor

iiison commented Aug 20, 2020

yep, we wouldn't stumble upon the single-multi issue if we start the props from new line. But, say, user doesn't run fix and make changes by themself. And makes the code look like(first prop is next to JSX Opening element):

<App foo 
  fooz
  bar baz
/>

should we show error in this case?

@iiison
Copy link
Contributor

iiison commented Aug 20, 2020

I'm implementing the "props to start from the new line" in the fix function for now. @ljharb | @jzabala Let me know if you think differently 😊

@ljharb
Copy link
Member

ljharb commented Aug 20, 2020

The priority is to make sure the tests are good, it's not all that important what the implementation is as long as it passes them :-)

iiison added a commit to iiison/eslint-plugin-react that referenced this issue Aug 26, 2020
@iiison
Copy link
Contributor

iiison commented Aug 26, 2020

@ljharb | @jzabala Created the PR: #2769

Let me know if that works 😊

@burtek

This comment has been minimized.

@0xdeafcafe

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Feb 20, 2022

Fixed by #3078.

@ljharb ljharb closed this as completed Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants