jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.98k stars 2.77k forks source link

Text at the end of JSX line is impossible #3094

Open fregante opened 3 years ago

fregante commented 3 years ago

Let's start simple, this is fine:

<span>(<b>Hi</b>)</span>

Let's say that Hi is actually super long so it needs to stay on its own line, for example:

<span>
    (
        <b>
            Nel mezzo del cammin di nostra vita mi ritrovai per una selva oscura, ché la diritta via era smarrita.
        </b>
    )
</span>

Fair enough… except:

  ✖  59:1   Expected indentation of 0 tabs but found 1.            @typescript-eslint/indent
  ✖  59:8   Ambiguous spacing before next element b                react/jsx-child-element-spacing
  ✖  59:8   Expected indentation of 0 tab characters but found 1.  react/jsx-indent
  ✖  61:1   Expected indentation of 0 tabs but found 1.            @typescript-eslint/indent
  ✖  61:12  Ambiguous spacing after previous element b             react/jsx-child-element-spacing

uh… let's try:

<span>
    (
    <b>
        Nel mezzo del cammin di nostra vita mi ritrovai per una selva oscura, ché la diritta via era smarrita.
    </b>
    )
</span>

ok, but still

  ✖  59:7   Ambiguous spacing before next element b     react/jsx-child-element-spacing
  ✖  61:11  Ambiguous spacing after previous element b  react/jsx-child-element-spacing

I suppose:

<span>
    (<b>
        Nel mezzo del cammin di nostra vita mi ritrovai per una selva oscura, ché la diritta via era smarrita.
    </b>)
</span>

But, a wild rule appears!

  ✖  60:7  Expected closing tag to match indentation of opening.  react/jsx-closing-tag-location

I'll spare you the rest of the steps, but the linter’s suggestion is:

<span>{`${'('}`}
    <b>
        Nel mezzo del cammin di nostra vita mi ritrovai per una selva oscura, ché la diritta via era smarrita.
    </b>)
</span>

Please show me the way. Is there any? What am I supposed to do? The last piece of code is the opposite of what I want a linter to help me write.

For the record, I'm using XO’s defaults: npx xo file.jsx

ljharb commented 3 years ago

The length of the line is largely irrelevant; if it's long prose there's not much value from putting it on its own line. I would expect this to work:

<span>
    {'('}<b>Nel mezzo del cammin di nostra vita mi ritrovai per una selva oscura, ché la diritta via era smarrita.</b>{')'}
</span>

or

<span>(<b>Nel mezzo del cammin di nostra vita mi ritrovai per una selva oscura, ché la diritta via era smarrita.</b>)</span>

The main problem here is that you're relying on the prose parens being right next to the <b /> tag - but if you have any whitespace between the ( and the <, you're risking there'll be rendered whitespace in the output. So, you should be omitting that whitespace, and using {'('} to indicate both to the linter and also to readers that the exact placement is important.

(I'm not familiar with xo's defaults; the airbnb config and prettier are the most commonly used ones by a very large margin, and I'm relatively sure they'd accept one of these)

fregante commented 3 years ago

I would expect this to work:

No such luck:

  ✖  58:2    Curly braces are unnecessary here.                     react/jsx-curly-brace-presence
  ✖  58:116  Curly braces are unnecessary here.                     react/jsx-curly-brace-presence

Leaving raw parens works, but it's not prose in the real situation, it's just a regular, long inline tag with classes

The length of the line is largely irrelevant;

For prose, maybe, but that's just an example. Is this line length ok? To me it's not, but it passes.

<span className="color-text-secondary">(<bdo className="css-truncate css-truncate-target" style={{maxWidth: '200px'}}>{names[userKey].name}</bdo>)</span>

I'm relatively sure they'd accept one of these

It doesn't with airbnb:

  2:2    error  Curly braces are unnecessary here                       react/jsx-curly-brace-presence
  2:7    error  `b` must be placed on a new line                        react/jsx-one-expression-per-line
  2:116  error  `{')'}` must be placed on a new line                    react/jsx-one-expression-per-line
  2:116  error  Curly braces are unnecessary here                       react/jsx-curly-brace-presence

I suggest trying any solutions before suggesting them because they all seem obvious, but none of the "good" ones pass. "Bad but passing lint" are:

fregante commented 3 years ago

What I've generally done in this situation is to split the JSX in multiple pieces, but I've never seen that in the wild:

const bdo = (
    <bdo className="css-truncate css-truncate-target" style={{maxWidth: '200px'}}>
        {names[userKey].name}
    </bdo>
);

<span className="color-text-secondary">(${bdo})</span>
ljharb commented 3 years ago

I didn’t have a playground to test it off hand; thanks for testing those. I agree that’s a gross combination of things to silence the linter.

I don’t think long lines are actually a problem, personally - i think limiting line length is a terribly ineffective tool to manage complexity - but you definitely should have an alternative way to write it that’s not that template literal thing.

I split up jsx like that all the time, but usually for clarity and not because the linter is making me jump through hoops.

If you have a suggestion of how any of the rules could be changed to accommodate this scenario without breaking existing use cases, I’m happy to hear it.

fregante commented 3 years ago

I'd find these reasonable:

<span className="color-text-secondary">
    (<bdo className="css-truncate css-truncate-target" style={{maxWidth: '200px'}}>
        {names[userKey].name}
    </bdo>)
</span>

👆 but react/jsx-closing-tag-location is found on the </bdo> line.

<span className="color-text-secondary">
    ${'('}
    <bdo className="css-truncate css-truncate-target" style={{maxWidth: '200px'}}>{names[userKey].name}</bdo>
    ${')'}
</span>

👆 somewhat readable and clear in intent, but react/jsx-curly-brace-presence complains on both.

Could either one of these be "fixed"? Could they understand the rule conflict and allow either solution? That is, "this is wrong, but the alternative would trigger rule XYZ, so I'll allow it."


In this specific case I think I'll go with the following, but of course if bdo were longer or had more children it'd be a problem again:

<span className="color-text-secondary">
    (<bdo className="css-truncate css-truncate-target" style={{maxWidth: '200px'}}>{names[userKey].name}</bdo>)
</span>
ljharb commented 3 years ago

Unfortunately rules have no way to know what other rules are enabled nor how they're configured.

fregante commented 3 years ago

I know, but it could do it anyway. They are all react rules.

ljharb commented 3 years ago

I'm not sure what you mean "could do it anyway" - eslint itself makes this impossible.

We could unconditionally allow a pattern, on the chance that it would be a conflict, but this would also allow undesirable code for those without this conflict or for those who are content with the long line.

fregante commented 3 years ago

We could unconditionally allow a pattern, on the chance that it would be a conflict

That's what I mean by "it," not "detect the other rules." It could also be an option on the rule itself, e.g. {allowIfConflictWithXYZ:true}

ljharb commented 3 years ago

An option would be fine; i'm not clear on what would be the most minimal change that also makes sense on its own.