Skip to content
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

[New] jsx-sort-props: add customPropsFirst to support custom props list for sorting #3853

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[New] jsx-sort-props: add customPropsFirst to support custom props list for sorting #3853

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 19 additions & 0 deletions docs/rules/jsx-sort-props.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Examples of **correct** code for this rule:
"ignoreCase": <boolean>,
"noSortAlphabetically": <boolean>,
"reservedFirst": <boolean>|<array<string>>,
"customPropsFirst": <array<string>>,
"locale": "auto" | "any valid locale"
}]
...
Expand Down Expand Up @@ -138,6 +139,24 @@ With `reservedFirst: ["key"]`, the following will **not** warn:
<Hello key={'uuid'} name="John" ref={johnRef} />
```

### `customPropsFirst`

This can only be an array option.

When `customPropsFirst` is defined, the specified custom props must be listed before all other props, but still respecting the alphabetical order:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When `customPropsFirst` is defined, the specified custom props must be listed before all other props, but still respecting the alphabetical order:
When `customPropsFirst` is defined, the specified custom props must be listed before all other props, but still respecting the configured order within the set of custom props:


```jsx
// 'jsx-sort-props': [1, { customPropsFirst: ["className", 'theme'] }]
<Hello className="flex" theme="light" name="John" />
```

If both `reservedFirst` and `customPropsFirst` are defined, reserved props are listed first, followed by custom props, and then all other props, but still respecting the alphabetical order:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If both `reservedFirst` and `customPropsFirst` are defined, reserved props are listed first, followed by custom props, and then all other props, but still respecting the alphabetical order:
If both `reservedFirst` and `customPropsFirst` are defined, reserved props are listed first, followed by custom props, and then all other props, but still respecting the configured order within each of those three groups:


```jsx
// 'jsx-sort-props': [1, { reservedFirst: true, customPropsFirst: ["className", 'theme'] }]
<Hello key={0} ref={johnRef} className="flex" theme="light" name="John" />
```

### `locale`

Defaults to `"auto"`, meaning, the locale of the current environment.
Expand Down
103 changes: 82 additions & 21 deletions lib/rules/jsx-sort-props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ function isMultilineProp(node) {

const messages = {
noUnreservedProps: 'A customized reserved first list must only contain a subset of React reserved props. Remove: {{unreservedWords}}',
listIsEmpty: 'A customized reserved first list must not be empty',
reservedListIsEmpty: 'A customized reserved first list must not be empty',
customPropsListIsEmpty: 'Custom props first list must not be empty',
listReservedPropsFirst: 'Reserved props must be listed before all other props',
listCustomPropsFirst: 'Custom props must be listed before all other props',
listCallbacksLast: 'Callbacks must be listed after all other props',
listShorthandFirst: 'Shorthand props must be listed before all other props',
listShorthandLast: 'Shorthand props must be listed after all other props',
Expand All @@ -45,7 +47,7 @@ const RESERVED_PROPS_LIST = [
'ref',
];

function isReservedPropName(name, list) {
function isPropNameInList(name, list) {
return list.indexOf(name) >= 0;
}

Expand All @@ -71,8 +73,8 @@ function contextCompare(a, b, options) {
}

if (options.reservedFirst) {
const aIsReserved = isReservedPropName(aProp, options.reservedList);
const bIsReserved = isReservedPropName(bProp, options.reservedList);
const aIsReserved = isPropNameInList(aProp, options.reservedList);
const bIsReserved = isPropNameInList(bProp, options.reservedList);
if (aIsReserved && !bIsReserved) {
return -1;
}
Expand All @@ -81,6 +83,17 @@ function contextCompare(a, b, options) {
}
}

if (options.customPropsList) {
const aIsCustom = isPropNameInList(aProp, options.customPropsList);
const bIsCustom = isPropNameInList(bProp, options.customPropsList);
if (aIsCustom && !bIsCustom) {
return -1;
}
if (!aIsCustom && bIsCustom) {
return 1;
}
}

if (options.callbacksLast) {
const aIsCallback = propTypesSortUtil.isCallbackPropName(aProp);
const bIsCallback = propTypesSortUtil.isCallbackPropName(bProp);
Expand Down Expand Up @@ -212,7 +225,7 @@ function getGroupsOfSortableAttributes(attributes, context) {
return sortableAttributeGroups;
}

function generateFixerFunction(node, context, reservedList) {
function generateFixerFunction(node, context, reservedList, customPropsList) {
const attributes = node.attributes.slice(0);
const configuration = context.options[0] || {};
const ignoreCase = configuration.ignoreCase || false;
Expand All @@ -222,11 +235,9 @@ function generateFixerFunction(node, context, reservedList) {
const multiline = configuration.multiline || 'ignore';
const noSortAlphabetically = configuration.noSortAlphabetically || false;
const reservedFirst = configuration.reservedFirst || false;
const customPropsFirst = configuration.customPropsFirst || false;
const locale = configuration.locale || 'auto';

// Sort props according to the context. Only supports ignoreCase.
// Since we cannot safely move JSXSpreadAttribute (due to potential variable overrides),
// we only consider groups of sortable attributes.
const options = {
ignoreCase,
callbacksLast,
Expand All @@ -236,8 +247,11 @@ function generateFixerFunction(node, context, reservedList) {
noSortAlphabetically,
reservedFirst,
reservedList,
customPropsFirst,
customPropsList,
locale,
};

const sortableAttributeGroups = getGroupsOfSortableAttributes(attributes, context);
const sortedAttributeGroups = sortableAttributeGroups
.slice(0)
Expand Down Expand Up @@ -284,14 +298,14 @@ function validateReservedFirstConfig(context, reservedFirst) {
if (reservedFirst) {
if (Array.isArray(reservedFirst)) {
// Only allow a subset of reserved words in customized lists
const nonReservedWords = reservedFirst.filter((word) => !isReservedPropName(
const nonReservedWords = reservedFirst.filter((word) => !isPropNameInList(
word,
RESERVED_PROPS_LIST
));

if (reservedFirst.length === 0) {
return function Report(decl) {
report(context, messages.listIsEmpty, 'listIsEmpty', {
report(context, messages.reservedListIsEmpty, 'reservedListIsEmpty', {
node: decl,
});
};
Expand All @@ -310,6 +324,27 @@ function validateReservedFirstConfig(context, reservedFirst) {
}
}

/**
* Checks if the `customPropsFirst` option is valid
* @param {Object} context The context of the rule
* @param {boolean | string[]} customPropsFirst The `customPropsFirst` option
* @return {Function | undefined} If an error is detected, a function to generate the error message, otherwise, `undefined`
*/
// eslint-disable-next-line consistent-return
function validateCustomPropsFirstConfig(context, customPropsFirst) {
if (customPropsFirst) {
if (Array.isArray(customPropsFirst)) {
if (customPropsFirst.length === 0) {
return function Report(decl) {
report(context, messages.customPropsListIsEmpty, 'customPropsListIsEmpty', {
node: decl,
});
};
}
}
}
}
Comment on lines +327 to +346
Copy link
Member

Choose a reason for hiding this comment

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

this should be handled by the schema, not by the plugin, and can be removed. (also an empty list is fine)


const reportedNodeAttributes = new WeakMap();
/**
* Check if the current node attribute has already been reported with the same error type
Expand All @@ -320,8 +355,9 @@ const reportedNodeAttributes = new WeakMap();
* @param {Object} node The parent node for the node attribute
* @param {Object} context The context of the rule
* @param {Array<String>} reservedList The list of reserved props
* @param {Array<String>} customPropsList The list of custom props
*/
function reportNodeAttribute(nodeAttribute, errorType, node, context, reservedList) {
function reportNodeAttribute(nodeAttribute, errorType, node, context, reservedList, customPropsList) {
const errors = reportedNodeAttributes.get(nodeAttribute) || [];

if (includes(errors, errorType)) {
Expand All @@ -334,7 +370,7 @@ function reportNodeAttribute(nodeAttribute, errorType, node, context, reservedLi

report(context, messages[errorType], errorType, {
node: nodeAttribute.name,
fix: generateFixerFunction(node, context, reservedList),
fix: generateFixerFunction(node, context, reservedList, customPropsList),
});
}

Expand Down Expand Up @@ -382,6 +418,9 @@ module.exports = {
reservedFirst: {
type: ['array', 'boolean'],
},
customPropsFirst: {
type: ['array', 'boolean'],
},
Comment on lines +421 to +423
Copy link
Member

Choose a reason for hiding this comment

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

true probably shouldn't be an allowed value here - also, the array should be limited to a unique list of strings.

Suggested change
customPropsFirst: {
type: ['array', 'boolean'],
},
customPropsFirst: {
oneOf: [
{
type: 'array',
uniqueItems: true,
items: { type: 'string' },
},
{
type: 'boolean',
enum: [false],
},
],
},

locale: {
type: 'string',
default: 'auto',
Expand All @@ -402,6 +441,9 @@ module.exports = {
const reservedFirst = configuration.reservedFirst || false;
const reservedFirstError = validateReservedFirstConfig(context, reservedFirst);
const reservedList = Array.isArray(reservedFirst) ? reservedFirst : RESERVED_PROPS_LIST;
const customPropsFirst = configuration.customPropsFirst || false;
const customPropsFirstError = validateCustomPropsFirstConfig(context, customPropsFirst);
const customPropsList = Array.isArray(customPropsFirst) ? customPropsFirst : [];
Comment on lines +444 to +446
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const customPropsFirst = configuration.customPropsFirst || false;
const customPropsFirstError = validateCustomPropsFirstConfig(context, customPropsFirst);
const customPropsList = Array.isArray(customPropsFirst) ? customPropsFirst : [];
const customPropsList = configuration.customPropsFirst;

no need to even ensure it's an array

const locale = configuration.locale || 'auto';

return {
Expand Down Expand Up @@ -436,14 +478,33 @@ module.exports = {
return memo;
}

const previousIsReserved = isReservedPropName(previousPropName, nodeReservedList);
const currentIsReserved = isReservedPropName(currentPropName, nodeReservedList);
const previousIsReserved = isPropNameInList(previousPropName, nodeReservedList);
const currentIsReserved = isPropNameInList(currentPropName, nodeReservedList);

if (previousIsReserved && !currentIsReserved) {
return decl;
}
if (!previousIsReserved && currentIsReserved) {
reportNodeAttribute(decl, 'listReservedPropsFirst', node, context, nodeReservedList);
reportNodeAttribute(decl, 'listReservedPropsFirst', node, context, nodeReservedList, customPropsList);

return memo;
}
}

if (customPropsFirst) {
if (customPropsFirstError) {
customPropsFirstError(decl);
return memo;
}

Comment on lines +495 to +499
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (customPropsFirstError) {
customPropsFirstError(decl);
return memo;
}

const previousIsCustom = isPropNameInList(propName(memo), customPropsList);
const currentIsCustom = isPropNameInList(propName(decl), customPropsList);

if (previousIsCustom && !currentIsCustom) {
return decl;
}
if (!previousIsCustom && currentIsCustom) {
reportNodeAttribute(decl, 'listCustomPropsFirst', node, context, nodeReservedList, customPropsList);

return memo;
}
Expand All @@ -456,7 +517,7 @@ module.exports = {
}
if (previousIsCallback && !currentIsCallback) {
// Encountered a non-callback prop after a callback prop
reportNodeAttribute(memo, 'listCallbacksLast', node, context, nodeReservedList);
reportNodeAttribute(memo, 'listCallbacksLast', node, context, nodeReservedList, customPropsList);

return memo;
}
Expand All @@ -467,7 +528,7 @@ module.exports = {
return decl;
}
if (!currentValue && previousValue) {
reportNodeAttribute(decl, 'listShorthandFirst', node, context, nodeReservedList);
reportNodeAttribute(decl, 'listShorthandFirst', node, context, nodeReservedList, customPropsList);

return memo;
}
Expand All @@ -478,7 +539,7 @@ module.exports = {
return decl;
}
if (currentValue && !previousValue) {
reportNodeAttribute(memo, 'listShorthandLast', node, context, nodeReservedList);
reportNodeAttribute(memo, 'listShorthandLast', node, context, nodeReservedList, customPropsList);

return memo;
}
Expand All @@ -493,7 +554,7 @@ module.exports = {
}
if (!previousIsMultiline && currentIsMultiline) {
// Encountered a non-multiline prop before a multiline prop
reportNodeAttribute(decl, 'listMultilineFirst', node, context, nodeReservedList);
reportNodeAttribute(decl, 'listMultilineFirst', node, context, nodeReservedList, customPropsList);

return memo;
}
Expand All @@ -504,7 +565,7 @@ module.exports = {
}
if (previousIsMultiline && !currentIsMultiline) {
// Encountered a non-multiline prop after a multiline prop
reportNodeAttribute(memo, 'listMultilineLast', node, context, nodeReservedList);
reportNodeAttribute(memo, 'listMultilineLast', node, context, nodeReservedList, customPropsList);

return memo;
}
Expand All @@ -518,7 +579,7 @@ module.exports = {
: previousPropName > currentPropName
)
) {
reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, nodeReservedList);
reportNodeAttribute(decl, 'sortPropsByAlpha', node, context, nodeReservedList, customPropsList);

return memo;
}
Expand Down
Loading
Loading