From a6e39d00e879876f768d5fd9c8bc09685169030b Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Mon, 6 Nov 2017 22:45:17 -0800 Subject: [PATCH 1/7] Failing tests for jsx-child-element-spacing proposal --- lib/rules/jsx-child-element-spacing.js | 19 +++++ tests/lib/rules/jsx-child-element-spacing.js | 73 ++++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 lib/rules/jsx-child-element-spacing.js create mode 100644 tests/lib/rules/jsx-child-element-spacing.js diff --git a/lib/rules/jsx-child-element-spacing.js b/lib/rules/jsx-child-element-spacing.js new file mode 100644 index 0000000000..ef8c179afe --- /dev/null +++ b/lib/rules/jsx-child-element-spacing.js @@ -0,0 +1,19 @@ +'use strict'; + +// TODO(pfhayes): Just a stub for now to get tests working +module.exports = { + meta: { + docs: {}, + schema: [ + { + type: 'object', + properties: {}, + default: {}, + additionalProperties: false + } + ] + }, + create: function (/* context */) { + return {}; + } +}; diff --git a/tests/lib/rules/jsx-child-element-spacing.js b/tests/lib/rules/jsx-child-element-spacing.js new file mode 100644 index 0000000000..82a5736fc2 --- /dev/null +++ b/tests/lib/rules/jsx-child-element-spacing.js @@ -0,0 +1,73 @@ +'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: ` + + foo + bar + + ` + }, { + code: ` + + foo + bar + + ` + }, { + code: ` + + foobarbaz + + ` + }, { + code: ` + + foo + {' '} + bar + {' '} + baz + + ` + }, { + code: ` + + foo{/* + */}bar{/* + */}baz + + ` + }], + + invalid: [{ + code: ` + + foo + bar + + `, + errors: ERROR_MESSAGE + }, { + code: ` + + bar + baz + + `, + errors: ERROR_MESSAGE + }] +}); From 0922c56b4acaef3b68d7b6a3cc17d9770e48858a Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Tue, 7 Nov 2017 21:25:58 -0800 Subject: [PATCH 2/7] Add some more test cases --- lib/rules/jsx-child-element-spacing.js | 1 - tests/lib/rules/jsx-child-element-spacing.js | 105 +++++++++++++++++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/lib/rules/jsx-child-element-spacing.js b/lib/rules/jsx-child-element-spacing.js index ef8c179afe..f99634a7e6 100644 --- a/lib/rules/jsx-child-element-spacing.js +++ b/lib/rules/jsx-child-element-spacing.js @@ -1,6 +1,5 @@ 'use strict'; -// TODO(pfhayes): Just a stub for now to get tests working module.exports = { meta: { docs: {}, diff --git a/tests/lib/rules/jsx-child-element-spacing.js b/tests/lib/rules/jsx-child-element-spacing.js index 82a5736fc2..14805c1918 100644 --- a/tests/lib/rules/jsx-child-element-spacing.js +++ b/tests/lib/rules/jsx-child-element-spacing.js @@ -14,6 +14,26 @@ const ERROR_MESSAGE = [{message: 'Ambiguous spacing between child elements.'}]; const ruleTester = new RuleTester({parserOptions}); ruleTester.run('jsx-child-element-spacing', rule, { valid: [{ + code: ` + + foo + + ` + }, { + code: ` + + bar + + ` + }, { + code: ` + + + nested + + + ` + }, { code: ` foo @@ -43,6 +63,22 @@ ruleTester.run('jsx-child-element-spacing', rule, { baz ` + }, { + code: ` + + foo + {' '}bar{' '} + baz + + ` + }, { + code: ` + + foo{' '} + bar + {' '}baz + + ` }, { code: ` @@ -51,6 +87,40 @@ ruleTester.run('jsx-child-element-spacing', rule, { */}baz ` + }, { + code: ` + + Please take a look at this link. + + ` + }, { + code: ` + + Please take a look at + {' '} + this link. + + ` + }, { + code: ` + +

A

+

B

+ + ` + }, { + code: ` + +

A

B

+ + ` + }, { + code: ` + + A + B + + ` }], invalid: [{ @@ -69,5 +139,40 @@ ruleTester.run('jsx-child-element-spacing', rule, { `, errors: ERROR_MESSAGE + }, { + code: ` + + {' '}bar + baz + + `, + errors: ERROR_MESSAGE + }, { + code: ` + + Please take a look at + this link. + + `, + errors: ERROR_MESSAGE + }, { + code: ` + + Here is + a link and here is + another + + `, + errors: ERROR_MESSAGE + }, { + code: ` + + + nested1 + nested2 + + + `, + errors: ERROR_MESSAGE }] }); From cc344806d576c76631b6e0a79ced9e96d8c33fc8 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Wed, 15 Nov 2017 15:01:00 -0800 Subject: [PATCH 3/7] Temp commit --- index.js | 5 +- lib/rules/jsx-child-element-spacing.js | 99 +++++++++++++++++++- tests/lib/rules/jsx-child-element-spacing.js | 23 +++-- 3 files changed, 113 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index a798443eb4..521b61752d 100644 --- a/index.js +++ b/index.js @@ -9,9 +9,10 @@ const allRules = { 'display-name': require('./lib/rules/display-name'), 'forbid-component-props': require('./lib/rules/forbid-component-props'), 'forbid-elements': require('./lib/rules/forbid-elements'), - 'forbid-prop-types': require('./lib/rules/forbid-prop-types'), 'forbid-foreign-prop-types': require('./lib/rules/forbid-foreign-prop-types'), + 'forbid-prop-types': require('./lib/rules/forbid-prop-types'), 'jsx-boolean-value': require('./lib/rules/jsx-boolean-value'), + 'jsx-child-element-spacing': require('./lib/rules/jsx-child-element-spacing'), 'jsx-closing-bracket-location': require('./lib/rules/jsx-closing-bracket-location'), 'jsx-closing-tag-location': require('./lib/rules/jsx-closing-tag-location'), 'jsx-curly-spacing': require('./lib/rules/jsx-curly-spacing'), @@ -23,12 +24,12 @@ const allRules = { 'jsx-indent-props': require('./lib/rules/jsx-indent-props'), 'jsx-key': require('./lib/rules/jsx-key'), 'jsx-max-props-per-line': require('./lib/rules/jsx-max-props-per-line'), - 'jsx-one-expression-per-line': require('./lib/rules/jsx-one-expression-per-line'), 'jsx-no-bind': require('./lib/rules/jsx-no-bind'), 'jsx-no-comment-textnodes': require('./lib/rules/jsx-no-comment-textnodes'), 'jsx-no-duplicate-props': require('./lib/rules/jsx-no-duplicate-props'), 'jsx-no-literals': require('./lib/rules/jsx-no-literals'), 'jsx-no-target-blank': require('./lib/rules/jsx-no-target-blank'), + 'jsx-one-expression-per-line': require('./lib/rules/jsx-one-expression-per-line'), 'jsx-no-undef': require('./lib/rules/jsx-no-undef'), 'jsx-curly-brace-presence': require('./lib/rules/jsx-curly-brace-presence'), 'jsx-pascal-case': require('./lib/rules/jsx-pascal-case'), diff --git a/lib/rules/jsx-child-element-spacing.js b/lib/rules/jsx-child-element-spacing.js index f99634a7e6..ab2999530d 100644 --- a/lib/rules/jsx-child-element-spacing.js +++ b/lib/rules/jsx-child-element-spacing.js @@ -1,8 +1,47 @@ 'use strict'; +const INLINE_ELEMENTS = [ + "a", + "abbr", + "acronym", + "b", + "bdo", + "big", + "br", + "button", + "cite", + "code", + "dfn", + "em", + "i", + "img", + "input", + "kbd", + "label", + "map", + "object", + "q", + "samp", + "script", + "select", + "small", + "span", + "strong", + "sub", + "sup", + "textarea", + "tt", + "var" +]; + module.exports = { meta: { - docs: {}, + docs: { + description: 'Ensures inline tags are not rendered without spaces between them', + category: 'Stylistic Issues', + recommended: false, + }, + fixable: false, schema: [ { type: 'object', @@ -12,7 +51,61 @@ module.exports = { } ] }, - create: function (/* context */) { - return {}; + create: function (context) { + const sourceCode = context.getSourceCode(); + + const isInlineElement = (node) => ( + node.type === 'JSXElement' && + node.openingElement && + node.openingElement.name && + node.openingElement.name.type === 'JSXIdentifier' && + INLINE_ELEMENTS.includes(node.openingElement.name.name) + ); + + return { + JSXElement: function(node) { + let lastChild = null; + let child = null; + let nextChild = null; + (node.children.concat([null])).forEach((nextChild) => { + if ( + (lastChild || nextChild) && + (!lastChild || isInlineElement(lastChild)) && + (child && child.type === 'Literal') && + (!nextChild || isInlineElement(nextChild)) && + true + ) { + if (lastChild && child.value.match(/^\s*\n\s*\S/)) { + context.report({ + node: child, + loc: child.loc, + message: 'CASE A: ' + child.value, + }); + } else if (lastChild && nextChild && child.value.match(/\n +$/)) { + context.report({ + node: child, + loc: child.loc, + message: 'CASE B: ' + child.value, + }); + } else if (nextChild && child.value.match(/\S\s*\n\s*$/)) { + context.report({ + node: child, + loc: child.loc, + message: 'CASE C: ' + child.value, + }); + } else if (lastChild && nextChild && child.value.match(/^ +\n/)) { + context.report({ + node: child, + loc: child.loc, + message: 'CASE D: ' + child.value, + }); + } + } + lastChild = child; + child = nextChild; + }); + Array.map + }, + }; } }; diff --git a/tests/lib/rules/jsx-child-element-spacing.js b/tests/lib/rules/jsx-child-element-spacing.js index 14805c1918..c6662ad77f 100644 --- a/tests/lib/rules/jsx-child-element-spacing.js +++ b/tests/lib/rules/jsx-child-element-spacing.js @@ -9,7 +9,8 @@ const parserOptions = { } }; -const ERROR_MESSAGE = [{message: 'Ambiguous spacing between child elements.'}]; +// const ERROR_MESSAGE = [{message: 'Ambiguous spacing between child elements.'}]; +const ERROR_MESSAGE = [{}]; const ruleTester = new RuleTester({parserOptions}); ruleTester.run('jsx-child-element-spacing', rule, { @@ -40,13 +41,6 @@ ruleTester.run('jsx-child-element-spacing', rule, { bar ` - }, { - code: ` - - foo - bar - - ` }, { code: ` @@ -131,6 +125,14 @@ ruleTester.run('jsx-child-element-spacing', rule, { `, errors: ERROR_MESSAGE + }, { + code: ` + + foo + bar + + `, + errors: ERROR_MESSAGE }, { code: ` @@ -163,7 +165,10 @@ ruleTester.run('jsx-child-element-spacing', rule, { another `, - errors: ERROR_MESSAGE + errors: [ + ERROR_MESSAGE, + ERROR_MESSAGE, + ], }, { code: ` From 9de7c73dacf37de5dd220345d485372307a9a5b7 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Wed, 15 Nov 2017 15:25:37 -0800 Subject: [PATCH 4/7] Cleanup --- lib/rules/jsx-child-element-spacing.js | 109 ++++++++++--------- tests/lib/rules/jsx-child-element-spacing.js | 4 +- 2 files changed, 58 insertions(+), 55 deletions(-) diff --git a/lib/rules/jsx-child-element-spacing.js b/lib/rules/jsx-child-element-spacing.js index ab2999530d..2f4aec3538 100644 --- a/lib/rules/jsx-child-element-spacing.js +++ b/lib/rules/jsx-child-element-spacing.js @@ -1,37 +1,37 @@ 'use strict'; const INLINE_ELEMENTS = [ - "a", - "abbr", - "acronym", - "b", - "bdo", - "big", - "br", - "button", - "cite", - "code", - "dfn", - "em", - "i", - "img", - "input", - "kbd", - "label", - "map", - "object", - "q", - "samp", - "script", - "select", - "small", - "span", - "strong", - "sub", - "sup", - "textarea", - "tt", - "var" + 'a', + 'abbr', + 'acronym', + 'b', + 'bdo', + 'big', + 'br', + 'button', + 'cite', + 'code', + 'dfn', + 'em', + 'i', + 'img', + 'input', + 'kbd', + 'label', + 'map', + 'object', + 'q', + 'samp', + 'script', + 'select', + 'small', + 'span', + 'strong', + 'sub', + 'sup', + 'textarea', + 'tt', + 'var' ]; module.exports = { @@ -39,7 +39,7 @@ module.exports = { docs: { description: 'Ensures inline tags are not rendered without spaces between them', category: 'Stylistic Issues', - recommended: false, + recommended: false }, fixable: false, schema: [ @@ -52,22 +52,28 @@ module.exports = { ] }, create: function (context) { - const sourceCode = context.getSourceCode(); - - const isInlineElement = (node) => ( - node.type === 'JSXElement' && + const elementName = node => ( node.openingElement && node.openingElement.name && node.openingElement.name.type === 'JSXIdentifier' && - INLINE_ELEMENTS.includes(node.openingElement.name.name) + node.openingElement.name.name + ); + + const isInlineElement = node => ( + node.type === 'JSXElement' && + INLINE_ELEMENTS.includes(elementName(node)) ); + const TRAILING_WHITESPACE_PATTERN = /\n +$/; + const LEADING_WHITESPACE_PATTERN = /^ +\n/; + const TEXT_FOLLOWING_ELEMENT_PATTERN = /^\s*\n\s*\S/; + const TEXT_PRECEDING_ELEMENT_PATTERN = /\S\s*\n\s*$/; + return { JSXElement: function(node) { let lastChild = null; let child = null; - let nextChild = null; - (node.children.concat([null])).forEach((nextChild) => { + (node.children.concat([null])).forEach(nextChild => { if ( (lastChild || nextChild) && (!lastChild || isInlineElement(lastChild)) && @@ -75,37 +81,34 @@ module.exports = { (!nextChild || isInlineElement(nextChild)) && true ) { - if (lastChild && child.value.match(/^\s*\n\s*\S/)) { + if ( + lastChild && + nextChild && + (child.value.match(LEADING_WHITESPACE_PATTERN) || child.value.match(TRAILING_WHITESPACE_PATTERN)) + ) { context.report({ node: child, loc: child.loc, - message: 'CASE A: ' + child.value, + message: `Ambiguous spacing between elements ${elementName(lastChild)} and ${elementName(nextChild)}` }); - } else if (lastChild && nextChild && child.value.match(/\n +$/)) { + } else if (lastChild && child.value.match(TEXT_FOLLOWING_ELEMENT_PATTERN)) { context.report({ node: child, loc: child.loc, - message: 'CASE B: ' + child.value, + message: `Ambiguous spacing after previous element ${elementName(lastChild)}` }); - } else if (nextChild && child.value.match(/\S\s*\n\s*$/)) { + } else if (nextChild && child.value.match(TEXT_PRECEDING_ELEMENT_PATTERN)) { context.report({ node: child, loc: child.loc, - message: 'CASE C: ' + child.value, - }); - } else if (lastChild && nextChild && child.value.match(/^ +\n/)) { - context.report({ - node: child, - loc: child.loc, - message: 'CASE D: ' + child.value, + message: `Ambiguous spacing before next element ${elementName(nextChild)}` }); } } lastChild = child; child = nextChild; }); - Array.map - }, + } }; } }; diff --git a/tests/lib/rules/jsx-child-element-spacing.js b/tests/lib/rules/jsx-child-element-spacing.js index c6662ad77f..fde9b31bac 100644 --- a/tests/lib/rules/jsx-child-element-spacing.js +++ b/tests/lib/rules/jsx-child-element-spacing.js @@ -167,8 +167,8 @@ ruleTester.run('jsx-child-element-spacing', rule, { `, errors: [ ERROR_MESSAGE, - ERROR_MESSAGE, - ], + ERROR_MESSAGE + ] }, { code: ` From 5b2e1d45e51fa6a44032d27ba36e4b44ac737af1 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Wed, 15 Nov 2017 15:57:16 -0800 Subject: [PATCH 5/7] Remove some cases --- lib/rules/jsx-child-element-spacing.js | 14 +---- tests/lib/rules/jsx-child-element-spacing.js | 63 ++++++++++++-------- 2 files changed, 39 insertions(+), 38 deletions(-) diff --git a/lib/rules/jsx-child-element-spacing.js b/lib/rules/jsx-child-element-spacing.js index 2f4aec3538..dc7f857af8 100644 --- a/lib/rules/jsx-child-element-spacing.js +++ b/lib/rules/jsx-child-element-spacing.js @@ -64,8 +64,6 @@ module.exports = { INLINE_ELEMENTS.includes(elementName(node)) ); - const TRAILING_WHITESPACE_PATTERN = /\n +$/; - const LEADING_WHITESPACE_PATTERN = /^ +\n/; const TEXT_FOLLOWING_ELEMENT_PATTERN = /^\s*\n\s*\S/; const TEXT_PRECEDING_ELEMENT_PATTERN = /\S\s*\n\s*$/; @@ -81,17 +79,7 @@ module.exports = { (!nextChild || isInlineElement(nextChild)) && true ) { - if ( - lastChild && - nextChild && - (child.value.match(LEADING_WHITESPACE_PATTERN) || child.value.match(TRAILING_WHITESPACE_PATTERN)) - ) { - context.report({ - node: child, - loc: child.loc, - message: `Ambiguous spacing between elements ${elementName(lastChild)} and ${elementName(nextChild)}` - }); - } else if (lastChild && child.value.match(TEXT_FOLLOWING_ELEMENT_PATTERN)) { + if (lastChild && child.value.match(TEXT_FOLLOWING_ELEMENT_PATTERN)) { context.report({ node: child, loc: child.loc, diff --git a/tests/lib/rules/jsx-child-element-spacing.js b/tests/lib/rules/jsx-child-element-spacing.js index fde9b31bac..51ebfcd671 100644 --- a/tests/lib/rules/jsx-child-element-spacing.js +++ b/tests/lib/rules/jsx-child-element-spacing.js @@ -9,9 +9,6 @@ const parserOptions = { } }; -// const ERROR_MESSAGE = [{message: 'Ambiguous spacing between child elements.'}]; -const ERROR_MESSAGE = [{}]; - const ruleTester = new RuleTester({parserOptions}); ruleTester.run('jsx-child-element-spacing', rule, { valid: [{ @@ -108,6 +105,22 @@ ruleTester.run('jsx-child-element-spacing', rule, {

A

B

 ` + }, { + code: ` + + foo + bar + + ` + }, { + code: ` + + + nested1 + nested2 + + + ` }, { code: ` @@ -124,15 +137,9 @@ ruleTester.run('jsx-child-element-spacing', rule, { bar `, - errors: ERROR_MESSAGE - }, { - code: ` - - foo - bar - - `, - errors: ERROR_MESSAGE + errors: [ + {message: 'Ambiguous spacing before next element a'} + ] }, { code: ` @@ -140,7 +147,9 @@ ruleTester.run('jsx-child-element-spacing', rule, { baz `, - errors: ERROR_MESSAGE + errors: [ + {message: 'Ambiguous spacing after previous element a'} + ] }, { code: ` @@ -148,7 +157,9 @@ ruleTester.run('jsx-child-element-spacing', rule, { baz `, - errors: ERROR_MESSAGE + errors: [ + {message: 'Ambiguous spacing after previous element a'} + ] }, { code: ` @@ -156,28 +167,30 @@ ruleTester.run('jsx-child-element-spacing', rule, { this link. `, - errors: ERROR_MESSAGE + errors: [ + {message: 'Ambiguous spacing before next element a'} + ] }, { code: ` - Here is - a link and here is - another + Some loops and some + if statements. `, errors: [ - ERROR_MESSAGE, - ERROR_MESSAGE + {message: 'Ambiguous spacing before next element code'} ] }, { code: ` - - nested1 - nested2 - + Here is + a link and here is + another `, - errors: ERROR_MESSAGE + errors: [ + {message: 'Ambiguous spacing before next element a'}, + {message: 'Ambiguous spacing before next element a'} + ] }] }); From c8ee19331217c9614c379a90871424dfcaea1e24 Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Wed, 15 Nov 2017 16:10:33 -0800 Subject: [PATCH 6/7] Avoid incldues --- lib/rules/jsx-child-element-spacing.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/jsx-child-element-spacing.js b/lib/rules/jsx-child-element-spacing.js index dc7f857af8..d39470559b 100644 --- a/lib/rules/jsx-child-element-spacing.js +++ b/lib/rules/jsx-child-element-spacing.js @@ -61,7 +61,7 @@ module.exports = { const isInlineElement = node => ( node.type === 'JSXElement' && - INLINE_ELEMENTS.includes(elementName(node)) + INLINE_ELEMENTS.indexOf(elementName(node)) !== -1 ); const TEXT_FOLLOWING_ELEMENT_PATTERN = /^\s*\n\s*\S/; From 7def79f4433bbe36328a38de6807a9ed58be232f Mon Sep 17 00:00:00 2001 From: Patrick Hayes Date: Tue, 21 Nov 2017 17:23:10 -0800 Subject: [PATCH 7/7] Review comments --- lib/rules/jsx-child-element-spacing.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/rules/jsx-child-element-spacing.js b/lib/rules/jsx-child-element-spacing.js index d39470559b..82d56917cb 100644 --- a/lib/rules/jsx-child-element-spacing.js +++ b/lib/rules/jsx-child-element-spacing.js @@ -1,6 +1,7 @@ 'use strict'; -const INLINE_ELEMENTS = [ +// This list is taken from https://developer.mozilla.org/en-US/docs/Web/HTML/Inline_elements +const INLINE_ELEMENTS = new Set([ 'a', 'abbr', 'acronym', @@ -32,7 +33,7 @@ const INLINE_ELEMENTS = [ 'textarea', 'tt', 'var' -]; +]); module.exports = { meta: { @@ -61,7 +62,7 @@ module.exports = { const isInlineElement = node => ( node.type === 'JSXElement' && - INLINE_ELEMENTS.indexOf(elementName(node)) !== -1 + INLINE_ELEMENTS.has(elementName(node)) ); const TEXT_FOLLOWING_ELEMENT_PATTERN = /^\s*\n\s*\S/;