diff --git a/packages/eslint-plugin/docs/rules/member-ordering.md b/packages/eslint-plugin/docs/rules/member-ordering.md index bfab8265260..aaece66adba 100644 --- a/packages/eslint-plugin/docs/rules/member-ordering.md +++ b/packages/eslint-plugin/docs/rules/member-ordering.md @@ -24,6 +24,7 @@ type OrderConfig = MemberType[] | SortedOrderConfig | 'never'; interface SortedOrderConfig { memberTypes?: MemberType[] | 'never'; + optionalityOrder?: 'optional-first' | 'required-first'; order: | 'alphabetically' | 'alphabetically-case-insensitive' @@ -44,9 +45,10 @@ You can configure `OrderConfig` options for: - **`interfaces`**?: override ordering specifically for interfaces - **`typeLiterals`**?: override ordering specifically for type literals -The `OrderConfig` settings for each kind of construct may configure sorting on one or both two levels: +The `OrderConfig` settings for each kind of construct may configure sorting on up to three levels: - **`memberTypes`**: organizing on member type groups such as methods vs. properties +- **`optionalityOrder`**: whether to put all optional members first or all required members first - **`order`**: organizing based on member names, such as alphabetically ### Groups @@ -911,6 +913,96 @@ interface Foo { } ``` +#### Sorting Optional Members First or Last + +The `optionalityOrder` option may be enabled to place all optional members in a group at the beginning or end of that group. + +This config places all optional members before all required members: + +```jsonc +// .eslintrc.json +{ + "rules": { + "@typescript-eslint/member-ordering": [ + "error", + { + "default": { + "optionalityOrder": "optional-first", + "order": "alphabetically" + } + } + ] + } +} +``` + + + +##### ❌ Incorrect + +```ts +interface Foo { + a: boolean; + b?: number; + c: string; +} +``` + +##### ✅ Correct + +```ts +interface Foo { + b?: number; + a: boolean; + c: string; +} +``` + + + +This config places all required members before all optional members: + +```jsonc +// .eslintrc.json +{ + "rules": { + "@typescript-eslint/member-ordering": [ + "error", + { + "default": { + "optionalityOrder": "required-first", + "order": "alphabetically" + } + } + ] + } +} +``` + + + +##### ❌ Incorrect + +```ts +interface Foo { + a: boolean; + b?: number; + c: string; +} +``` + +##### ✅ Correct + +```ts +interface Foo { + a: boolean; + c: string; + b?: number; +} +``` + + + ## All Supported Options ### Member Types (Granular Form) diff --git a/packages/eslint-plugin/src/rules/member-ordering.ts b/packages/eslint-plugin/src/rules/member-ordering.ts index c92af2650ce..87022f16f4f 100644 --- a/packages/eslint-plugin/src/rules/member-ordering.ts +++ b/packages/eslint-plugin/src/rules/member-ordering.ts @@ -4,7 +4,10 @@ import naturalCompare from 'natural-compare-lite'; import * as util from '../util'; -export type MessageIds = 'incorrectGroupOrder' | 'incorrectOrder'; +export type MessageIds = + | 'incorrectGroupOrder' + | 'incorrectOrder' + | 'incorrectRequiredMembersOrder'; type MemberKind = | 'call-signature' @@ -47,12 +50,15 @@ type Order = AlphabeticalOrder | 'as-written'; interface SortedOrderConfig { memberTypes?: MemberType[] | 'never'; + optionalityOrder?: OptionalityOrder; order: Order; } type OrderConfig = MemberType[] | SortedOrderConfig | 'never'; type Member = TSESTree.ClassElement | TSESTree.TypeElement; +type OptionalityOrder = 'optional-first' | 'required-first'; + export type Options = [ { default?: OrderConfig; @@ -101,6 +107,10 @@ const objectConfig = (memberTypes: MemberType[]): JSONSchema.JSONSchema4 => ({ 'natural-case-insensitive', ], }, + optionalityOrder: { + type: 'string', + enum: ['optional-first', 'required-first'], + }, }, additionalProperties: false, }); @@ -405,6 +415,26 @@ function getMemberName( } } +/** + * Returns true if the member is optional based on the member type. + * + * @param node the node to be evaluated. + * + * @returns Whether the member is optional, or false if it cannot be optional at all. + */ +function isMemberOptional(node: Member): boolean { + switch (node.type) { + case AST_NODE_TYPES.TSPropertySignature: + case AST_NODE_TYPES.TSMethodSignature: + case AST_NODE_TYPES.TSAbstractPropertyDefinition: + case AST_NODE_TYPES.PropertyDefinition: + case AST_NODE_TYPES.TSAbstractMethodDefinition: + case AST_NODE_TYPES.MethodDefinition: + return !!node.optional; + } + return false; +} + /** * Gets the calculated rank using the provided method definition. * The algorithm is as follows: @@ -561,6 +591,7 @@ export default util.createRule({ 'Member {{member}} should be declared before member {{beforeMember}}.', incorrectGroupOrder: 'Member {{name}} should be declared before all {{rank}} definitions.', + incorrectRequiredMembersOrder: `Member {{member}} should be declared after all {{optionalOrRequired}} members.`, }, schema: [ { @@ -725,6 +756,59 @@ export default util.createRule({ } } + /** + * Checks if the order of optional and required members is correct based + * on the given 'required' parameter. + * + * @param members Members to be validated. + * @param optionalityOrder Where to place optional members, if not intermixed. + * + * @return True if all required and optional members are correctly sorted. + */ + function checkRequiredOrder( + members: Member[], + optionalityOrder: OptionalityOrder | undefined, + ): boolean { + const switchIndex = members.findIndex( + (member, i) => + i && isMemberOptional(member) !== isMemberOptional(members[i - 1]), + ); + + const report = (member: Member): void => + context.report({ + messageId: 'incorrectRequiredMembersOrder', + loc: member.loc, + data: { + member: getMemberName(member, context.getSourceCode()), + optionalOrRequired: + optionalityOrder === 'optional-first' ? 'required' : 'optional', + }, + }); + + // if the optionality of the first item is correct (based on optionalityOrder) + // then the first 0 inclusive to switchIndex exclusive members all + // have the correct optionality + if ( + isMemberOptional(members[0]) !== + (optionalityOrder === 'required-first') + ) { + report(members[0]); + return false; + } + + for (let i = switchIndex + 1; i < members.length; i++) { + if ( + isMemberOptional(members[i]) !== + isMemberOptional(members[switchIndex]) + ) { + report(members[switchIndex]); + return false; + } + } + + return true; + } + /** * Validates if all members are correctly sorted. * @@ -743,33 +827,62 @@ export default util.createRule({ // Standardize config let order: Order | undefined; - let memberTypes; + let memberTypes: string | MemberType[] | undefined; + let optionalityOrder: OptionalityOrder | undefined; + + // returns true if everything is good and false if an error was reported + const checkOrder = (memberSet: Member[]): boolean => { + const hasAlphaSort = !!(order && order !== 'as-written'); + + // Check order + if (Array.isArray(memberTypes)) { + const grouped = checkGroupSort( + memberSet, + memberTypes, + supportsModifiers, + ); + + if (grouped === null) { + return false; + } + + if (hasAlphaSort) { + return !grouped.some( + groupMember => + !checkAlphaSort(groupMember, order as AlphabeticalOrder), + ); + } + } else if (hasAlphaSort) { + return checkAlphaSort(memberSet, order as AlphabeticalOrder); + } + + return true; + }; if (Array.isArray(orderConfig)) { memberTypes = orderConfig; } else { order = orderConfig.order; memberTypes = orderConfig.memberTypes; + optionalityOrder = orderConfig.optionalityOrder; } - const hasAlphaSort = !!(order && order !== 'as-written'); + if (!optionalityOrder) { + checkOrder(members); + return; + } - // Check order - if (Array.isArray(memberTypes)) { - const grouped = checkGroupSort(members, memberTypes, supportsModifiers); + const switchIndex = members.findIndex( + (member, i) => + i && isMemberOptional(member) !== isMemberOptional(members[i - 1]), + ); - if (grouped === null) { + if (switchIndex !== -1) { + if (!checkRequiredOrder(members, optionalityOrder)) { return; } - - if (hasAlphaSort) { - grouped.some( - groupMember => - !checkAlphaSort(groupMember, order as AlphabeticalOrder), - ); - } - } else if (hasAlphaSort) { - checkAlphaSort(members, order as AlphabeticalOrder); + checkOrder(members.slice(0, switchIndex)); + checkOrder(members.slice(switchIndex)); } } diff --git a/packages/eslint-plugin/src/util/misc.ts b/packages/eslint-plugin/src/util/misc.ts index 2bdc8ee0f59..351c94a6361 100644 --- a/packages/eslint-plugin/src/util/misc.ts +++ b/packages/eslint-plugin/src/util/misc.ts @@ -180,6 +180,29 @@ function formatWordList(words: string[]): string { return [words.slice(0, -1).join(', '), words.slice(-1)[0]].join(' and '); } +/** + * Iterates the array in reverse and returns the index of the first element it + * finds which passes the predicate function. + * + * @returns Returns the index of the element if it finds it or -1 otherwise. + */ +function findLastIndex( + members: T[], + predicate: (member: T) => boolean | undefined | null, +): number { + let idx = members.length - 1; + + while (idx >= 0) { + const valid = predicate(members[idx]); + if (valid) { + return idx; + } + idx--; + } + + return -1; +} + export { arrayGroupByToMap, arraysAreEqual, @@ -194,4 +217,5 @@ export { MemberNameType, RequireKeys, upperCaseFirst, + findLastIndex, }; diff --git a/packages/eslint-plugin/tests/rules/member-ordering/member-ordering-optionalMembers.test.ts b/packages/eslint-plugin/tests/rules/member-ordering/member-ordering-optionalMembers.test.ts new file mode 100644 index 00000000000..9c72fc7322a --- /dev/null +++ b/packages/eslint-plugin/tests/rules/member-ordering/member-ordering-optionalMembers.test.ts @@ -0,0 +1,458 @@ +import type { TSESLint } from '@typescript-eslint/utils'; + +import type { MessageIds, Options } from '../../../src/rules/member-ordering'; +import rule from '../../../src/rules/member-ordering'; +import { RuleTester } from '../../RuleTester'; + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', +}); + +const grouped: TSESLint.RunTests = { + valid: [ + // optionalityOrder - optional-first + { + code: ` +interface X { + c: string; + b?: string; + d?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +interface X { + b?: string; + c?: string; + d?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'as-written', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +interface X { + b: string; + c: string; + d: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'as-written', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +class X { + c: string; + d: string; + ['a']?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +class X { + c: string; + public static d: string; + public static ['a']?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +class X { + a: string; + static {} + b: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +class X { + a: string; + [i: number]: string; + b?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +interface X { + a: string; + [i?: number]: string; + b?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + { + code: ` +interface X { + a: string; + (a: number): string; + new (i: number): string; + b?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + }, + // optionalityOrder - required-first + { + code: ` +interface X { + b?: string; + d?: string; + c: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'required-first', + }, + }, + ], + }, + { + code: ` +interface X { + b?: string; + c?: string; + d?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'as-written', + optionalityOrder: 'required-first', + }, + }, + ], + }, + { + code: ` +interface X { + b: string; + c: string; + d: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'as-written', + optionalityOrder: 'required-first', + }, + }, + ], + }, + { + code: ` +class X { + ['c']?: string; + a: string; + b: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'required-first', + }, + }, + ], + }, + ], + // optionalityOrder - optional-first + invalid: [ + { + code: ` +interface X { + m: string; + d?: string; + b?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'optional-first', + }, + }, + ], + errors: [ + { + messageId: 'incorrectOrder', + line: 5, + column: 3, + }, + ], + }, + { + code: ` +interface X { + a: string; + b?: string; + c: string; +} + `, + options: [ + { + default: { + memberTypes: ['call-signature', 'field', 'method'], + order: 'as-written', + optionalityOrder: 'optional-first', + }, + }, + ], + errors: [ + { + messageId: 'incorrectRequiredMembersOrder', + line: 4, + column: 3, + data: { + member: 'b', + optionalOrRequired: 'required', + }, + }, + ], + }, + { + code: ` +class X { + a?: string; + static {} + b?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'as-written', + optionalityOrder: 'optional-first', + }, + }, + ], + errors: [ + { + messageId: 'incorrectRequiredMembersOrder', + line: 3, + column: 3, + data: { + member: 'a', + optionalOrRequired: 'required', + }, + }, + ], + }, + // optionalityOrder - required-first + { + code: ` +interface X { + d?: string; + b?: string; + m: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'alphabetically', + optionalityOrder: 'required-first', + }, + }, + ], + errors: [ + { + messageId: 'incorrectOrder', + line: 4, + column: 3, + }, + ], + }, + { + code: ` +interface X { + a?: string; + b: string; + c?: string; +} + `, + options: [ + { + default: { + memberTypes: ['call-signature', 'field', 'method'], + order: 'as-written', + optionalityOrder: 'required-first', + }, + }, + ], + errors: [ + { + messageId: 'incorrectRequiredMembersOrder', + line: 4, + column: 3, + data: { + member: 'b', + optionalOrRequired: 'optional', + }, + }, + ], + }, + { + code: ` +class Test { + a?: string; + b?: string; + f: string; + c?: string; + d?: string; + g: string; + h: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'as-written', + optionalityOrder: 'required-first', + }, + }, + ], + errors: [ + { + messageId: 'incorrectRequiredMembersOrder', + line: 5, + column: 3, + data: { + member: 'f', + optionalOrRequired: 'optional', + }, + }, + ], + }, + { + code: ` +class Test { + a: string; + b: string; + f?: string; + c?: string; + d?: string; +} + `, + options: [ + { + default: { + memberTypes: 'never', + order: 'as-written', + optionalityOrder: 'required-first', + }, + }, + ], + errors: [ + { + messageId: 'incorrectRequiredMembersOrder', + line: 3, + column: 3, + data: { + member: 'a', + optionalOrRequired: 'optional', + }, + }, + ], + }, + ], +}; + +ruleTester.run('member-ordering-required', rule, grouped); diff --git a/packages/eslint-plugin/tests/util/misc.test.ts b/packages/eslint-plugin/tests/util/misc.test.ts index c4291f00d9f..6eae810eb62 100644 --- a/packages/eslint-plugin/tests/util/misc.test.ts +++ b/packages/eslint-plugin/tests/util/misc.test.ts @@ -23,3 +23,17 @@ describe('formatWordList', () => { ); }); }); + +describe('findLastIndex', () => { + it('returns -1 if there are no elements to iterate over', () => { + expect(misc.findLastIndex([], () => true)).toBe(-1); + }); + + it('returns the index of the last element if predicate just returns true for all values', () => { + expect(misc.findLastIndex([1, 2, 3], () => true)).toBe(2); + }); + + it('returns the index of the last occurance of a duplicate element', () => { + expect(misc.findLastIndex([1, 2, 3, 3, 5], n => n === 3)).toBe(3); + }); +});