Proposal: jsx-child-element-spacing proposal

[pfhayes]

Failing tests to illustrate use cases for #1515

[pfhayes]

@ljharb

[pfhayes]

Open to feedback on

• Does this make sense, and would it be a desirable rule to add
• Naming and preferred language for error messages

[ljharb]

what would the output be if you wanted this autofixed?

[pfhayes]

I think as you pointed out in the original issue, the intent is ambiguous so autofixing may not be feasible.

If we were to autofix, replacing with the comment form would maintain the semantic without too much disruption (though would perhaps be less aesthetically pleasing)

I would advocate for no automatic action since, in my experience, the developer most often intended to add a space, though I could be convinced otherwise

[ljharb]

Could you add a bunch more valid and invalid examples? That would help evaluate the usefulness of the rule.

[pfhayes]

Thanks @ljharb - I've added some more test cases that cover some different case. Please let me know if that helps or if I can provide cases of another type

[ljharb]

why is this an error? are you going to keep a list of inline-by-default elements? What about when the display is changed via css?

[pfhayes]

It would be an error because it would render as nested1nested2 which is unlikely to be the developers intent

We certainly could just use the complete list of inline elements here: https://developer.mozilla.org/en-US/docs/Web/HTML/Inline_elements

Certainly it would not be feasible to capture changes to display by CSS. I wouldn't be overly concerned about it because...

• If the developer uses CSS to make a block element inline, then the rule will not catch this. However without this rule they are equally likely not to catch this. I would also expect that, since the developer is already modifying the default layout, they are likely to be visually inspecting the results manually to look for inconsistencies
• If the developer uses CSS to make an inline element into a block, then the rule may error unnecessarily. However the error can be silenced by adding either an explicit space or a comment form, as above.

[ljharb]

Unless there's already a package that keeps the list of inline elements up to date, i'd be hesitant to hardcode it in this package.

[pfhayes]

A fair concern - here's one that I found: https://www.npmjs.com/package/block-elements (and it's less popular cousin https://www.npmjs.com/package/inline-elements)

Worth noting that React, when tackling a similar problem, opted to just hardcode a list of relevant tags

https://github.com/facebook/react/blob/17aa4d4682aa5c6fdbd3fb0d65e2d69769fe2d61/packages/react-dom/src/client/validateDOMNesting.js#L29

Another alternative would be not to do any special-casing on tag type and treat any adjacent tags as problematic, though I suspect that would get annoying pretty fast with adjacent divs.

Another concern - custom React classes for which it would be infeasible to detect the display type statically. This leads me to believe the rule should only report cases where we know the tag is inline - I think this covers a lot of common cases (like tags in text)

[ljharb]

The real ideal would be if we could use React's list directly.

[pfhayes]

Unfortunately, they are not the same use case

Note: this does not catch all invalid nesting, nor does it try to (as it's
  // not clear what practical benefit doing so provides); instead, we warn only
  // for cases where the parser will give a parse tree differing from what React
  // intended. For example, <b><div></div></b> is invalid but we don't warn
  // because it still parses correctly; we do warn for other cases like nested
  // <p> tags where the beginning of the second element implicitly closes the
  // first, causing a confusing mess.

[pfhayes]

I've updated this PR to reduce its scope.

Originally I was hoping to catch cases like

<p>
  <a href="https://google.com>link 1</a>
  <a href="https://google.com>link 2</a>
</p>

the original draft of this rule searched for cases like that. Ultimately there were just too many false positives (such as a's, span's, button's that used CSS to distinguish between them).

By reducing the scope to just search for

<p>
  Here is a
  <a href="https://google.com>link</a>
</p>

the rule became much more useful and has very few false positives. I ran this on about 60k lines of React code and the results were

Legitimate errors caught: 35
Not an error, but original was ambiguous and could be improved: 17
False Positives: 2

The majority of the error cases were a's, code's, strong's appearing inside paragraphs. The majority of the ambiguous cases were checkboxes appearing next to text. The false positives were that were separately styled with CSS

This suggests to me this is a useful rule and I submit it for your consideration

cc @ljharb

[ljharb]

Thanks, I think this LGTM.

[ljharb]

could you add a comment here that links to the source of this list?

[ljharb]

should this be made into a Set instead?

[pfhayes]

comments addressed @ljharb