Skip to content

Commit c14e209

Browse files
ROSSROSALESljharb
ROSSROSALES
authored and
ljharb
committed
[Fix] jsx-sort-props: sorted attributes now respect comments
Fixes #2366.
1 parent 1656707 commit c14e209

File tree

3 files changed

+324
-31
lines changed

3 files changed

+324
-31
lines changed

‎CHANGELOG.md

+2
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
2323
* [`display-name`], component detection: fix false positive for HOF returning only nulls and literals ([#3305][] @golopot)
2424
* [`jsx-no-target-blank`]: False negative when rel attribute is assigned using ConditionalExpression ([#3332][] @V2dha)
2525
* [`jsx-no-leaked-render`]: autofix nested "&&" logical expressions ([#3353][] @hduprat)
26+
* [`jsx-sort-props`]: sorted attributes now respect comments ([#3358][] @ROSSROSALES)
2627

2728
### Changed
2829
* [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
5051
[#3362]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3362
5152
[#3361]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3361
5253
[#3359]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3359
54+
[#3358]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3358
5355
[#3355]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3355
5456
[#3353]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3353
5557
[#3351]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3351

‎lib/rules/jsx-sort-props.js

+93-29
Original file line numberDiff line numberDiff line change
@@ -46,10 +46,27 @@ function isReservedPropName(name, list) {
4646
return list.indexOf(name) >= 0;
4747
}
4848

49+
let attributeMap;
50+
// attributeMap = [endrange, true||false if comment in between nodes exists, it needs to be sorted to end]
51+
52+
function shouldSortToEnd(node) {
53+
const attr = attributeMap.get(node);
54+
return !!attr && !!attr[1];
55+
}
56+
4957
function contextCompare(a, b, options) {
5058
let aProp = propName(a);
5159
let bProp = propName(b);
5260

61+
const aSortToEnd = shouldSortToEnd(a);
62+
const bSortToEnd = shouldSortToEnd(b);
63+
if (aSortToEnd && !bSortToEnd) {
64+
return 1;
65+
}
66+
if (!aSortToEnd && bSortToEnd) {
67+
return -1;
68+
}
69+
5370
if (options.reservedFirst) {
5471
const aIsReserved = isReservedPropName(aProp, options.reservedList);
5572
const bIsReserved = isReservedPropName(bProp, options.reservedList);
@@ -118,32 +135,79 @@ function contextCompare(a, b, options) {
118135
* Create an array of arrays where each subarray is composed of attributes
119136
* that are considered sortable.
120137
* @param {Array<JSXSpreadAttribute|JSXAttribute>} attributes
138+
* @param {Object} context The context of the rule
121139
* @return {Array<Array<JSXAttribute>>}
122140
*/
123-
function getGroupsOfSortableAttributes(attributes) {
141+
function getGroupsOfSortableAttributes(attributes, context) {
142+
const sourceCode = context.getSourceCode();
143+
124144
const sortableAttributeGroups = [];
125145
let groupCount = 0;
146+
function addtoSortableAttributeGroups(attribute) {
147+
sortableAttributeGroups[groupCount - 1].push(attribute);
148+
}
149+
126150
for (let i = 0; i < attributes.length; i++) {
151+
const attribute = attributes[i];
152+
const nextAttribute = attributes[i + 1];
153+
const attributeline = attribute.loc.start.line;
154+
let comment = [];
155+
try {
156+
comment = sourceCode.getCommentsAfter(attribute);
157+
} catch (e) { /**/ }
127158
const lastAttr = attributes[i - 1];
159+
const attrIsSpread = attribute.type === 'JSXSpreadAttribute';
160+
128161
// If we have no groups or if the last attribute was JSXSpreadAttribute
129162
// then we start a new group. Append attributes to the group until we
130163
// come across another JSXSpreadAttribute or exhaust the array.
131164
if (
132165
!lastAttr
133-
|| (lastAttr.type === 'JSXSpreadAttribute'
134-
&& attributes[i].type !== 'JSXSpreadAttribute')
166+
|| (lastAttr.type === 'JSXSpreadAttribute' && !attrIsSpread)
135167
) {
136168
groupCount += 1;
137169
sortableAttributeGroups[groupCount - 1] = [];
138170
}
139-
if (attributes[i].type !== 'JSXSpreadAttribute') {
140-
sortableAttributeGroups[groupCount - 1].push(attributes[i]);
171+
if (!attrIsSpread) {
172+
if (comment.length === 0) {
173+
attributeMap.set(attribute, [attribute.range[1], false]);
174+
addtoSortableAttributeGroups(attribute);
175+
} else {
176+
const firstComment = comment[0];
177+
const commentline = firstComment.loc.start.line;
178+
if (comment.length === 1) {
179+
if (attributeline + 1 === commentline && nextAttribute) {
180+
attributeMap.set(attribute, [nextAttribute.range[1], true]);
181+
addtoSortableAttributeGroups(attribute);
182+
i += 1;
183+
} else if (attributeline === commentline) {
184+
if (firstComment.type === 'Block') {
185+
attributeMap.set(attribute, [nextAttribute.range[1], true]);
186+
i += 1;
187+
} else {
188+
attributeMap.set(attribute, [firstComment.range[1], false]);
189+
}
190+
addtoSortableAttributeGroups(attribute);
191+
}
192+
} else if (comment.length > 1 && attributeline + 1 === comment[1].loc.start.line && nextAttribute) {
193+
const commentNextAttribute = sourceCode.getCommentsAfter(nextAttribute);
194+
attributeMap.set(attribute, [nextAttribute.range[1], true]);
195+
if (
196+
commentNextAttribute.length === 1
197+
&& nextAttribute.loc.start.line === commentNextAttribute[0].loc.start.line
198+
) {
199+
attributeMap.set(attribute, [commentNextAttribute[0].range[1], true]);
200+
}
201+
addtoSortableAttributeGroups(attribute);
202+
i += 1;
203+
}
204+
}
141205
}
142206
}
143207
return sortableAttributeGroups;
144208
}
145209

146-
const generateFixerFunction = (node, context, reservedList) => {
210+
function generateFixerFunction(node, context, reservedList) {
147211
const sourceCode = context.getSourceCode();
148212
const attributes = node.attributes.slice(0);
149213
const configuration = context.options[0] || {};
@@ -170,7 +234,7 @@ const generateFixerFunction = (node, context, reservedList) => {
170234
reservedList,
171235
locale,
172236
};
173-
const sortableAttributeGroups = getGroupsOfSortableAttributes(attributes);
237+
const sortableAttributeGroups = getGroupsOfSortableAttributes(attributes, context);
174238
const sortedAttributeGroups = sortableAttributeGroups
175239
.slice(0)
176240
.map((group) => group.slice(0).sort((a, b) => contextCompare(a, b, options)));
@@ -179,13 +243,13 @@ const generateFixerFunction = (node, context, reservedList) => {
179243
const fixers = [];
180244
let source = sourceCode.getText();
181245

182-
// Replace each unsorted attribute with the sorted one.
183246
sortableAttributeGroups.forEach((sortableGroup, ii) => {
184247
sortableGroup.forEach((attr, jj) => {
185248
const sortedAttr = sortedAttributeGroups[ii][jj];
186-
const sortedAttrText = sourceCode.getText(sortedAttr);
249+
const sortedAttrText = source.substring(sortedAttr.range[0], attributeMap.get(sortedAttr)[0]);
250+
const attrrangeEnd = attributeMap.get(attr)[0];
187251
fixers.push({
188-
range: [attr.range[0], attr.range[1]],
252+
range: [attr.range[0], attrrangeEnd],
189253
text: sortedAttrText,
190254
});
191255
});
@@ -202,7 +266,7 @@ const generateFixerFunction = (node, context, reservedList) => {
202266

203267
return fixer.replaceTextRange([rangeStart, rangeEnd], source.substr(rangeStart, rangeEnd - rangeStart));
204268
};
205-
};
269+
}
206270

207271
/**
208272
* Checks if the `reservedFirst` option is valid
@@ -331,15 +395,17 @@ module.exports = {
331395
const noSortAlphabetically = configuration.noSortAlphabetically || false;
332396
const reservedFirst = configuration.reservedFirst || false;
333397
const reservedFirstError = validateReservedFirstConfig(context, reservedFirst);
334-
let reservedList = Array.isArray(reservedFirst) ? reservedFirst : RESERVED_PROPS_LIST;
398+
const reservedList = Array.isArray(reservedFirst) ? reservedFirst : RESERVED_PROPS_LIST;
335399
const locale = configuration.locale || 'auto';
336400

337401
return {
402+
Program() {
403+
attributeMap = new WeakMap();
404+
},
405+
338406
JSXOpeningElement(node) {
339407
// `dangerouslySetInnerHTML` is only "reserved" on DOM components
340-
if (reservedFirst && !jsxUtil.isDOMComponent(node)) {
341-
reservedList = reservedList.filter((prop) => prop !== 'dangerouslySetInnerHTML');
342-
}
408+
const nodeReservedList = reservedFirst && !jsxUtil.isDOMComponent(node) ? reservedList.filter((prop) => prop !== 'dangerouslySetInnerHTML') : reservedList;
343409

344410
node.attributes.reduce((memo, decl, idx, attrs) => {
345411
if (decl.type === 'JSXSpreadAttribute') {
@@ -352,8 +418,6 @@ module.exports = {
352418
const currentValue = decl.value;
353419
const previousIsCallback = isCallbackPropName(previousPropName);
354420
const currentIsCallback = isCallbackPropName(currentPropName);
355-
const previousIsMultiline = isMultilineProp(memo);
356-
const currentIsMultiline = isMultilineProp(decl);
357421

358422
if (ignoreCase) {
359423
previousPropName = previousPropName.toLowerCase();
@@ -366,14 +430,14 @@ module.exports = {
366430
return memo;
367431
}
368432

369-
const previousIsReserved = isReservedPropName(previousPropName, reservedList);
370-
const currentIsReserved = isReservedPropName(currentPropName, reservedList);
433+
const previousIsReserved = isReservedPropName(previousPropName, nodeReservedList);
434+
const currentIsReserved = isReservedPropName(currentPropName, nodeReservedList);
371435

372436
if (previousIsReserved && !currentIsReserved) {
373437
return decl;
374438
}
375439
if (!previousIsReserved && currentIsReserved) {
376-
reportNodeAttribute(decl, 'listReservedPropsFirst', node, context, reservedList);
440+
reportNodeAttribute(decl, 'listReservedPropsFirst', node, context, nodeReservedList);
377441

378442
return memo;
379443
}
@@ -386,7 +450,7 @@ module.exports = {
386450
}
387451
if (previousIsCallback && !currentIsCallback) {
388452
// Encountered a non-callback prop after a callback prop
389-
reportNodeAttribute(memo, 'listCallbacksLast', node, context, reservedList);
453+
reportNodeAttribute(memo, 'listCallbacksLast', node, context, nodeReservedList);
390454

391455
return memo;
392456
}
@@ -397,7 +461,7 @@ module.exports = {
397461
return decl;
398462
}
399463
if (!currentValue && previousValue) {
400-
reportNodeAttribute(decl, 'listShorthandFirst', node, context, reservedList);
464+
reportNodeAttribute(decl, 'listShorthandFirst', node, context, nodeReservedList);
401465

402466
return memo;
403467
}
@@ -408,33 +472,33 @@ module.exports = {
408472
return decl;
409473
}
410474
if (currentValue && !previousValue) {
411-
reportNodeAttribute(memo, 'listShorthandLast', node, context, reservedList);
475+
reportNodeAttribute(memo, 'listShorthandLast', node, context, nodeReservedList);
412476

413477
return memo;
414478
}
415479
}
416480

481+
const previousIsMultiline = isMultilineProp(memo);
482+
const currentIsMultiline = isMultilineProp(decl);
417483
if (multiline === 'first') {
418484
if (previousIsMultiline && !currentIsMultiline) {
419485
// Exiting the multiline prop section
420486
return decl;
421487
}
422488
if (!previousIsMultiline && currentIsMultiline) {
423489
// Encountered a non-multiline prop before a multiline prop
424-
reportNodeAttribute(decl, 'listMultilineFirst', node, context, reservedList);
490+
reportNodeAttribute(decl, 'listMultilineFirst', node, context, nodeReservedList);
425491

426492
return memo;
427493
}
428-
}
429-
430-
if (multiline === 'last') {
494+
} else if (multiline === 'last') {
431495
if (!previousIsMultiline && currentIsMultiline) {
432496
// Entering the multiline prop section
433497
return decl;
434498
}
435499
if (previousIsMultiline && !currentIsMultiline) {
436500
// Encountered a non-multiline prop after a multiline prop
437-
reportNodeAttribute(memo, 'listMultilineLast', node, context, reservedList);
501+
reportNodeAttribute(memo, 'listMultilineLast', node, context, nodeReservedList);
438502

439503
return memo;
440504
}
@@ -448,7 +512,7 @@ module.exports = {
448512
: previousPropName > currentPropName
449513
)
450514
) {
451-
reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, reservedList);
515+
reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, nodeReservedList);
452516

453517
return memo;
454518
}

0 commit comments

Comments
 (0)