From c14e209dad8f8d48f1aae7106d9169b001e6e412 Mon Sep 17 00:00:00 2001 From: Ross <52366381+ROSSROSALES@users.noreply.github.com> Date: Tue, 16 Aug 2022 21:26:59 +0000 Subject: [PATCH] [Fix] `jsx-sort-props`: sorted attributes now respect comments Fixes #2366. --- CHANGELOG.md | 2 + lib/rules/jsx-sort-props.js | 122 ++++++++++++---- tests/lib/rules/jsx-sort-props.js | 231 +++++++++++++++++++++++++++++- 3 files changed, 324 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4a2132bd0..9b241aca33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange * [`display-name`], component detection: fix false positive for HOF returning only nulls and literals ([#3305][] @golopot) * [`jsx-no-target-blank`]: False negative when rel attribute is assigned using ConditionalExpression ([#3332][] @V2dha) * [`jsx-no-leaked-render`]: autofix nested "&&" logical expressions ([#3353][] @hduprat) +* [`jsx-sort-props`]: sorted attributes now respect comments ([#3358][] @ROSSROSALES) ### Changed * [Refactor] [`jsx-indent-props`]: improved readability of the checkNodesIndent function ([#3315][] @caroline223) @@ -50,6 +51,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange [#3362]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3362 [#3361]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3361 [#3359]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3359 +[#3358]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3358 [#3355]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3355 [#3353]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3353 [#3351]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3351 diff --git a/lib/rules/jsx-sort-props.js b/lib/rules/jsx-sort-props.js index ea2e8c88d3..dea9085035 100644 --- a/lib/rules/jsx-sort-props.js +++ b/lib/rules/jsx-sort-props.js @@ -46,10 +46,27 @@ function isReservedPropName(name, list) { return list.indexOf(name) >= 0; } +let attributeMap; +// attributeMap = [endrange, true||false if comment in between nodes exists, it needs to be sorted to end] + +function shouldSortToEnd(node) { + const attr = attributeMap.get(node); + return !!attr && !!attr[1]; +} + function contextCompare(a, b, options) { let aProp = propName(a); let bProp = propName(b); + const aSortToEnd = shouldSortToEnd(a); + const bSortToEnd = shouldSortToEnd(b); + if (aSortToEnd && !bSortToEnd) { + return 1; + } + if (!aSortToEnd && bSortToEnd) { + return -1; + } + if (options.reservedFirst) { const aIsReserved = isReservedPropName(aProp, options.reservedList); const bIsReserved = isReservedPropName(bProp, options.reservedList); @@ -118,32 +135,79 @@ function contextCompare(a, b, options) { * Create an array of arrays where each subarray is composed of attributes * that are considered sortable. * @param {Array} attributes + * @param {Object} context The context of the rule * @return {Array>} */ -function getGroupsOfSortableAttributes(attributes) { +function getGroupsOfSortableAttributes(attributes, context) { + const sourceCode = context.getSourceCode(); + const sortableAttributeGroups = []; let groupCount = 0; + function addtoSortableAttributeGroups(attribute) { + sortableAttributeGroups[groupCount - 1].push(attribute); + } + for (let i = 0; i < attributes.length; i++) { + const attribute = attributes[i]; + const nextAttribute = attributes[i + 1]; + const attributeline = attribute.loc.start.line; + let comment = []; + try { + comment = sourceCode.getCommentsAfter(attribute); + } catch (e) { /**/ } const lastAttr = attributes[i - 1]; + const attrIsSpread = attribute.type === 'JSXSpreadAttribute'; + // If we have no groups or if the last attribute was JSXSpreadAttribute // then we start a new group. Append attributes to the group until we // come across another JSXSpreadAttribute or exhaust the array. if ( !lastAttr - || (lastAttr.type === 'JSXSpreadAttribute' - && attributes[i].type !== 'JSXSpreadAttribute') + || (lastAttr.type === 'JSXSpreadAttribute' && !attrIsSpread) ) { groupCount += 1; sortableAttributeGroups[groupCount - 1] = []; } - if (attributes[i].type !== 'JSXSpreadAttribute') { - sortableAttributeGroups[groupCount - 1].push(attributes[i]); + if (!attrIsSpread) { + if (comment.length === 0) { + attributeMap.set(attribute, [attribute.range[1], false]); + addtoSortableAttributeGroups(attribute); + } else { + const firstComment = comment[0]; + const commentline = firstComment.loc.start.line; + if (comment.length === 1) { + if (attributeline + 1 === commentline && nextAttribute) { + attributeMap.set(attribute, [nextAttribute.range[1], true]); + addtoSortableAttributeGroups(attribute); + i += 1; + } else if (attributeline === commentline) { + if (firstComment.type === 'Block') { + attributeMap.set(attribute, [nextAttribute.range[1], true]); + i += 1; + } else { + attributeMap.set(attribute, [firstComment.range[1], false]); + } + addtoSortableAttributeGroups(attribute); + } + } else if (comment.length > 1 && attributeline + 1 === comment[1].loc.start.line && nextAttribute) { + const commentNextAttribute = sourceCode.getCommentsAfter(nextAttribute); + attributeMap.set(attribute, [nextAttribute.range[1], true]); + if ( + commentNextAttribute.length === 1 + && nextAttribute.loc.start.line === commentNextAttribute[0].loc.start.line + ) { + attributeMap.set(attribute, [commentNextAttribute[0].range[1], true]); + } + addtoSortableAttributeGroups(attribute); + i += 1; + } + } } } return sortableAttributeGroups; } -const generateFixerFunction = (node, context, reservedList) => { +function generateFixerFunction(node, context, reservedList) { const sourceCode = context.getSourceCode(); const attributes = node.attributes.slice(0); const configuration = context.options[0] || {}; @@ -170,7 +234,7 @@ const generateFixerFunction = (node, context, reservedList) => { reservedList, locale, }; - const sortableAttributeGroups = getGroupsOfSortableAttributes(attributes); + const sortableAttributeGroups = getGroupsOfSortableAttributes(attributes, context); const sortedAttributeGroups = sortableAttributeGroups .slice(0) .map((group) => group.slice(0).sort((a, b) => contextCompare(a, b, options))); @@ -179,13 +243,13 @@ const generateFixerFunction = (node, context, reservedList) => { const fixers = []; let source = sourceCode.getText(); - // Replace each unsorted attribute with the sorted one. sortableAttributeGroups.forEach((sortableGroup, ii) => { sortableGroup.forEach((attr, jj) => { const sortedAttr = sortedAttributeGroups[ii][jj]; - const sortedAttrText = sourceCode.getText(sortedAttr); + const sortedAttrText = source.substring(sortedAttr.range[0], attributeMap.get(sortedAttr)[0]); + const attrrangeEnd = attributeMap.get(attr)[0]; fixers.push({ - range: [attr.range[0], attr.range[1]], + range: [attr.range[0], attrrangeEnd], text: sortedAttrText, }); }); @@ -202,7 +266,7 @@ const generateFixerFunction = (node, context, reservedList) => { return fixer.replaceTextRange([rangeStart, rangeEnd], source.substr(rangeStart, rangeEnd - rangeStart)); }; -}; +} /** * Checks if the `reservedFirst` option is valid @@ -331,15 +395,17 @@ module.exports = { const noSortAlphabetically = configuration.noSortAlphabetically || false; const reservedFirst = configuration.reservedFirst || false; const reservedFirstError = validateReservedFirstConfig(context, reservedFirst); - let reservedList = Array.isArray(reservedFirst) ? reservedFirst : RESERVED_PROPS_LIST; + const reservedList = Array.isArray(reservedFirst) ? reservedFirst : RESERVED_PROPS_LIST; const locale = configuration.locale || 'auto'; return { + Program() { + attributeMap = new WeakMap(); + }, + JSXOpeningElement(node) { // `dangerouslySetInnerHTML` is only "reserved" on DOM components - if (reservedFirst && !jsxUtil.isDOMComponent(node)) { - reservedList = reservedList.filter((prop) => prop !== 'dangerouslySetInnerHTML'); - } + const nodeReservedList = reservedFirst && !jsxUtil.isDOMComponent(node) ? reservedList.filter((prop) => prop !== 'dangerouslySetInnerHTML') : reservedList; node.attributes.reduce((memo, decl, idx, attrs) => { if (decl.type === 'JSXSpreadAttribute') { @@ -352,8 +418,6 @@ module.exports = { const currentValue = decl.value; const previousIsCallback = isCallbackPropName(previousPropName); const currentIsCallback = isCallbackPropName(currentPropName); - const previousIsMultiline = isMultilineProp(memo); - const currentIsMultiline = isMultilineProp(decl); if (ignoreCase) { previousPropName = previousPropName.toLowerCase(); @@ -366,14 +430,14 @@ module.exports = { return memo; } - const previousIsReserved = isReservedPropName(previousPropName, reservedList); - const currentIsReserved = isReservedPropName(currentPropName, reservedList); + const previousIsReserved = isReservedPropName(previousPropName, nodeReservedList); + const currentIsReserved = isReservedPropName(currentPropName, nodeReservedList); if (previousIsReserved && !currentIsReserved) { return decl; } if (!previousIsReserved && currentIsReserved) { - reportNodeAttribute(decl, 'listReservedPropsFirst', node, context, reservedList); + reportNodeAttribute(decl, 'listReservedPropsFirst', node, context, nodeReservedList); return memo; } @@ -386,7 +450,7 @@ module.exports = { } if (previousIsCallback && !currentIsCallback) { // Encountered a non-callback prop after a callback prop - reportNodeAttribute(memo, 'listCallbacksLast', node, context, reservedList); + reportNodeAttribute(memo, 'listCallbacksLast', node, context, nodeReservedList); return memo; } @@ -397,7 +461,7 @@ module.exports = { return decl; } if (!currentValue && previousValue) { - reportNodeAttribute(decl, 'listShorthandFirst', node, context, reservedList); + reportNodeAttribute(decl, 'listShorthandFirst', node, context, nodeReservedList); return memo; } @@ -408,12 +472,14 @@ module.exports = { return decl; } if (currentValue && !previousValue) { - reportNodeAttribute(memo, 'listShorthandLast', node, context, reservedList); + reportNodeAttribute(memo, 'listShorthandLast', node, context, nodeReservedList); return memo; } } + const previousIsMultiline = isMultilineProp(memo); + const currentIsMultiline = isMultilineProp(decl); if (multiline === 'first') { if (previousIsMultiline && !currentIsMultiline) { // Exiting the multiline prop section @@ -421,20 +487,18 @@ module.exports = { } if (!previousIsMultiline && currentIsMultiline) { // Encountered a non-multiline prop before a multiline prop - reportNodeAttribute(decl, 'listMultilineFirst', node, context, reservedList); + reportNodeAttribute(decl, 'listMultilineFirst', node, context, nodeReservedList); return memo; } - } - - if (multiline === 'last') { + } else if (multiline === 'last') { if (!previousIsMultiline && currentIsMultiline) { // Entering the multiline prop section return decl; } if (previousIsMultiline && !currentIsMultiline) { // Encountered a non-multiline prop after a multiline prop - reportNodeAttribute(memo, 'listMultilineLast', node, context, reservedList); + reportNodeAttribute(memo, 'listMultilineLast', node, context, nodeReservedList); return memo; } @@ -448,7 +512,7 @@ module.exports = { : previousPropName > currentPropName ) ) { - reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, reservedList); + reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, nodeReservedList); return memo; } diff --git a/tests/lib/rules/jsx-sort-props.js b/tests/lib/rules/jsx-sort-props.js index 3abcbb8233..3c74dba74c 100644 --- a/tests/lib/rules/jsx-sort-props.js +++ b/tests/lib/rules/jsx-sort-props.js @@ -11,6 +11,7 @@ const RuleTester = require('eslint').RuleTester; const semver = require('semver'); +const eslintPkg = require('eslint/package.json'); const rule = require('../../../lib/rules/jsx-sort-props'); const parsers = require('../../helpers/parsers'); @@ -297,7 +298,7 @@ ruleTester.run('jsx-sort-props', rule, { options: [{ locale: 'sk-SK' }], } : [] )), - invalid: parsers.all([ + invalid: parsers.all([].concat( { code: ';', errors: [expectedError], @@ -821,5 +822,231 @@ ruleTester.run('jsx-sort-props', rule, { }, ], }, - ]), + semver.satisfies(eslintPkg.version, '> 3') ? { + code: ` + + `, + output: ` + + `, + errors: [ + { + messageId: 'sortPropsByAlpha', + line: 6, + }, + { + messageId: 'sortPropsByAlpha', + line: 8, + }, + { + messageId: 'sortPropsByAlpha', + line: 9, + }, + { + messageId: 'sortPropsByAlpha', + line: 10, + }, + { + messageId: 'sortPropsByAlpha', + line: 11, + }, + ], + } : [], + semver.satisfies(eslintPkg.version, '> 3') ? { + code: ` + + `, + output: ` + + `, + errors: [ + { + messageId: 'sortPropsByAlpha', + line: 6, + }, + { + messageId: 'sortPropsByAlpha', + line: 7, + }, + { + messageId: 'sortPropsByAlpha', + line: 8, + }, + { + messageId: 'sortPropsByAlpha', + line: 9, + }, + { + messageId: 'sortPropsByAlpha', + line: 10, + }, + { + messageId: 'sortPropsByAlpha', + line: 11, + }, + ], + } : [], + semver.satisfies(eslintPkg.version, '> 3') ? { + code: ` + + `, + output: ` + + `, + errors: [ + { + messageId: 'sortPropsByAlpha', + line: 5, + }, + { + messageId: 'sortPropsByAlpha', + line: 7, + }, + { + messageId: 'sortPropsByAlpha', + line: 8, + }, + { + messageId: 'sortPropsByAlpha', + line: 10, + }, + { + messageId: 'sortPropsByAlpha', + line: 11, + }, + { + messageId: 'sortPropsByAlpha', + line: 12, + }, + ], + } : [], + semver.satisfies(eslintPkg.version, '> 3') ? { + code: ` + + `, + output: ` + + `, + errors: [ + { + messageId: 'sortPropsByAlpha', + line: 8, + }, + { + messageId: 'sortPropsByAlpha', + line: 10, + }, + { + messageId: 'sortPropsByAlpha', + line: 11, + }, + { + messageId: 'sortPropsByAlpha', + line: 12, + }, + ], + } : [], + semver.satisfies(eslintPkg.version, '> 3') ? { + code: ` + + `, + output: ` + + `, + errors: [ + { + messageId: 'sortPropsByAlpha', + line: 2, + }, + { + messageId: 'sortPropsByAlpha', + line: 2, + }, + ], + } : [] + )), });