Skip to content

Proposal: jsx-child-element-spacing proposal #1519

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

Merged
merged 8 commits into from
Dec 1, 2017
Merged

Proposal: jsx-child-element-spacing proposal #1519

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a6e39d0
Failing tests for jsx-child-element-spacing proposal
pfhayes Nov 7, 2017
0922c56
Add some more test cases
pfhayes Nov 8, 2017
cc34480
Temp commit
pfhayes Nov 15, 2017
9de7c73
Cleanup
pfhayes Nov 15, 2017
5b2e1d4
Remove some cases
pfhayes Nov 15, 2017
bda68ce
Merge remote-tracking branch 'upstream/master' into spaces
pfhayes Nov 16, 2017
c8ee193
Avoid incldues
pfhayes Nov 16, 2017
7def79f
Review comments
pfhayes Nov 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/rules/jsx-child-element-spacing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

module.exports = {
meta: {
docs: {},
schema: [
{
type: 'object',
properties: {},
default: {},
additionalProperties: false
}
]
},
create: function (/* context */) {
return {};
}
};
178 changes: 178 additions & 0 deletions tests/lib/rules/jsx-child-element-spacing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
'use strict';

const rule = require('../../../lib/rules/jsx-child-element-spacing');
const RuleTester = require('eslint').RuleTester;
const parserOptions = {
sourceType: 'module',
ecmaFeatures: {
jsx: true
}
};

const ERROR_MESSAGE = [{message: 'Ambiguous spacing between child elements.'}];

const ruleTester = new RuleTester({parserOptions});
ruleTester.run('jsx-child-element-spacing', rule, {
valid: [{
code: `
<App>
foo
</App>
`
}, {
code: `
<App>
<a>bar</a>
</App>
`
}, {
code: `
<App>
<a>
<b>nested</b>
</a>
</App>
`
}, {
code: `
<App>
foo
bar
</App>
`
}, {
code: `
<App>
<a>foo</a>
<a>bar</a>
</App>
`
}, {
code: `
<App>
foo<a>bar</a>baz
</App>
`
}, {
code: `
<App>
foo
{' '}
<a>bar</a>
{' '}
baz
</App>
`
}, {
code: `
<App>
foo
{' '}<a>bar</a>{' '}
baz
</App>
`
}, {
code: `
<App>
foo{' '}
<a>bar</a>
{' '}baz
</App>
`
}, {
code: `
<App>
foo{/*
*/}<a>bar</a>{/*
*/}baz
</App>
`
}, {
code: `
<App>
Please take a look at <a href="https://js.org">this link</a>.
</App>
`
}, {
code: `
<App>
Please take a look at
{' '}
<a href="https://js.org">this link</a>.
</App>
`
}, {
code: `
<App>
<p>A</p>
<p>B</p>
</App>
`
}, {
code: `
<App>
<p>A</p><p>B</p>
</App>
`
}, {
code: `
<App>
A
B
</App>
`
}],

invalid: [{
code: `
<App>
foo
<a>bar</a>
Copy link
Member

Choose a reason for hiding this comment

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

what would the output be if you wanted this autofixed?

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 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

</App>
`,
errors: ERROR_MESSAGE
}, {
code: `
<App>
<a>bar</a>
baz
</App>
`,
errors: ERROR_MESSAGE
}, {
code: `
<App>
{' '}<a>bar</a>
baz
</App>
`,
errors: ERROR_MESSAGE
}, {
code: `
<App>
Please take a look at
<a href="https://js.org">this link</a>.
</App>
`,
errors: ERROR_MESSAGE
}, {
code: `
<App>
Here is
<a href="https://js.org">a link</a> and here is
<a href="https://js.org">another</a>
</App>
`,
errors: ERROR_MESSAGE
}, {
code: `
<App>
<a>
<b>nested1</b>
<b>nested2</b>
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

</a>
</App>
`,
errors: ERROR_MESSAGE
}]
});