jsx-eslint/eslint-plugin-reactPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star9.2k

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

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Draft

saimonkat wants to merge1 commit intojsx-eslint:master

base:master

Choose a base branch

fromsaimonkat:jsx-sort-props-extra-props-list

Draft

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

saimonkat wants to merge1 commit intojsx-eslint:masterfromsaimonkat:jsx-sort-props-extra-props-list

Conversation

Copy link

saimonkat commentedNov 18, 2024

This PR brings a newcustomPropsFirst option forjsx-sort-props to support custom user's props list for sorting

Refering to:#3851
Resolving issues:#3175 #3639 #3193

It's my first time writing an ESLint rule option, so please let me know what could be better. Also, this is my first time writing tests. I expect we'll have to write a lot more than I've come up with so far, but let's start with this

[New]jsx-sort-props: addcustomPropsFirst to support custom prop…

09676ab

…s list for sorting

ljharb requested changes

Nov 18, 2024

View reviewed changes

Copy link

Member

ljharb left a comment

Choose a reason for hiding this comment

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

looks pretty solid for a first start :-)

let's be sure to duplicate a number of the examples that cover the permutations of sort ordering, showing that with a custom props list (of similar examples), the ordering is enforced within that group as well.

docs/rules/jsx-sort-props.md


		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

ljharbNov 18, 2024

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 thealphabetical order:
	When`customPropsFirst` is defined, the specified custom props must be listed before all other props, but still respecting theconfigured order within the set of custom props:

docs/rules/jsx-sort-props.md

		<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

ljharbNov 18, 2024

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 thealphabetical 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 theconfigured order within each of those three groups:

lib/rules/jsx-sort-props.js

Comment on lines +421 to +423

		customPropsFirst: {
		type: ['array', 'boolean'],
		},

Copy link

Member

ljharbNov 18, 2024

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],
	},
	],
	},

lib/rules/jsx-sort-props.js

Comment on lines +327 to +346

		/**
		* 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,
		});
		};
		}
		}
		}
		}

Copy link

Member

ljharbNov 18, 2024

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)

lib/rules/jsx-sort-props.js

Comment on lines +444 to +446

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

Copy link

Member

ljharbNov 18, 2024

Choose a reason for hiding this comment

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

Suggested change

	constcustomPropsFirst=configuration.customPropsFirst\|\|false;
	constcustomPropsFirstError=validateCustomPropsFirstConfig(context,customPropsFirst);
	constcustomPropsList=Array.isArray(customPropsFirst) ?customPropsFirst :[];
	constcustomPropsList=configuration.customPropsFirst;