jsx-first-prop-new-line: autofix

[snowypowers]

Hi I have made an autofix for the rule jsx-first-prop-new-line for a fork and I thought it would be useful to have it on the main repo.

I am aware and monitoring #883. Will update the autofix after that is approved! Meanwhile, PR for visibility and to start the process :)

[ljharb]

Thanks! Could you rebase, so as to remove all merge commits?

[ljharb]

instead of adding an additional option, could we just choose any arbitrary indentation style, and rely on the indent autofixer to correct it?

[ljharb]

If we did want to add the option, I think we'd want to make it be an object so that it's extensible in the future.

[snowypowers]

If we are to rely on indent autofixer wouldnt we require 2 runs of lint?
Anyway the current implementation does default to a specific style (2 spaces). Most of the indent fixing code is actually referenced and stripped down from jsx-indent.

To make it clearer, I could word it such that autofix has a optional arg for indents but does work without it.

PS: I think I should change the default back to 4 spaces so to be inline with indent defaults

[ljharb]

Yes, it might require two passes of --fix but I think that's OK.

And yes, the default should match whatever indent defaults to.

[revisionfour]

Can we go ahead and merge this in? Where are we with this? I added an issue in the repo that this autofix needs to be added and it looks like its right here waiting.

[ljharb]

This is waiting on @snowypowers removing the indentation option.

(also, s/utilise/utilize)

[snowypowers]

• Removed all traces of indentation argument
• utilize
• Added a simple warning line in readme that the fixer will require a second run of lint to fix indentation

[ljharb]

i'd like a few other collabs to weigh in, but LGTM!

[snowypowers]

Rebased on top of #883. No major changes except for adding outputs to test cases.
However, I would like to take this moment to point out that the current code uses a foreach loop:

        if (
          (configuration === 'multiline' && isMultilineJSX(node)) ||
          (configuration === 'multiline-multiprop' && isMultilineJSX(node) && node.attributes.length > 1) ||
          (configuration === 'always')
        ) {
          node.attributes.forEach(function(decl) {
            //code to verify and fix EACH PROP
         }

This results in multiple errors being thrown in fail cases with multiple props:

options: ['always']

<Foo propOne="one" propTwo="two"/>

Seeing that this rule is only concerned with amending the position of the first prop, should the function be only checking for the first prop and throwing error if the first prop fails?

Possible fix:

// We check for at least one prop to ensure we can safely use node.attributes[0] inside
        if (
          (configuration === 'multiline' && isMultilineJSX(node) && node.attributes.length > 0) ||
          (configuration === 'multiline-multiprop' && isMultilineJSX(node) && node.attributes.length > 1) ||
          (configuration === 'always' && node.attributes.length > 0)
        ) {
          var decl = node.attributes[0];
         //Code to verify and fix decl

[ljharb]

@snowypowers while i think it makes sense to only throw for the first error as long as the autofixer results in no remaining cases of the error.

[snowypowers]

Included the fix for multiple props on the first line causing lint to throw 1 error per prop, causing AssertionError. First Invalid test case amended to include such a scenario.

Hope that is all :)

[revisionfour]

To be clear, if we can get this completed and merged and the Readme updated with the --fix command included
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-first-prop-new-line.md

Then I'd like to donate $50 American to @snowypowers. I can Paypal or Venmo this if you shoot an email to leec85@gmail.com with the email you want it delivered to.

That would take care of this 930

My team could also greatly benefit from closing out 929 with a --fix command for jsx-max-props-per-line.

I'd donate another $50 American for whoever can get this working, completed and merged with the readme updated.

Have any questions, find me on twitter. Go team open source.
@revisionfour

[ljharb]

shouldn't we only return true inside the if? otherwise it will only ever run on the first value of node.attributes - is that intended?

[snowypowers]

Yes, it is intentional. This rule only concerns the first prop for each React element so we should not be checking anything else after the first element.

[ljharb]

I guess the some then is just a nicer way of doing [0] and then having to check if it's defined?

[snowypowers]

yeah, its somewhat cleaner than adding && node.attributes.length > 0. Matter of preference.

[ljharb]

sounds good 👍

[ljharb]

I'll give this a day or two to be reviewed by other collabs, then I'll merge it in.

[revisionfour]

$50 donated to @snowypowers.

Thanks Yak Jun Xiang!

If you or anyone else is up for it, 929 has a $50 bounty on it.

Go team open source!

[snowypowers]

Hey thanks @revisionfour for the generous donation! Glad to know that my code will be utilised immediately!