Skip to content

Commit

Permalink
[Fix] jsx-sort-props: sorted attributes now respect comments
Browse files Browse the repository at this point in the history 
Fixes #2366.
  • Loading branch information
@ROSSROSALES @ljharb
ROSSROSALES authored and ljharb committed Aug 16, 2022
1 parent 1656707 commit c14e209
Show file tree
Hide file tree
Showing 3 changed files with 324 additions and 31 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
122 changes: 93 additions & 29 deletions lib/rules/jsx-sort-props.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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<JSXSpreadAttribute|JSXAttribute>} attributes
* @param {Object} context The context of the rule
* @return {Array<Array<JSXAttribute>>}
*/
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] || {};
Expand All @@ -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)));
Expand All @@ -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,
});
});
Expand All @@ -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
Expand Down Expand Up @@ -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') {
Expand All @@ -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();
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -408,33 +472,33 @@ 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
return decl;
}
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;
}
Expand All @@ -448,7 +512,7 @@ module.exports = {
: previousPropName > currentPropName
)
) {
reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, reservedList);
reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, nodeReservedList);

return memo;
}
Expand Down
Loading

0 comments on commit c14e209

Please sign in to comment.