8000 [member-ordering] Add ability to sort enums · Issue #4859 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]

Skip to content
8000

[member-ordering] Add ability to sort enums #4859

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Zamiell opened this issue Apr 25, 2022 · 12 comments
Closed

[member-ordering] Add ability to sort enums #4859

Zamiell opened this issue Apr 25, 2022 · 12 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@Zamiell
Copy link
Contributor
Zamiell commented Apr 25, 2022

Hello, and thanks for your excellent linter.

Currently, the @typescript-eslint/member-ordering rule supports sorting classes, interfaces, and type literals.

But it doesn't support enums. Can someone please add enums? It seems like it would be a super-quick addition.

For example, with the following ESLint configuration:

  rules: {
    "@typescript-eslint/member-ordering": [
      "warn",
      {
        enums: {
          order: "alphabetically",
        },
      },
    ],
  }

ESLint should warn me that the following enum is not in alphabetical order:

enum Fruit {
  Banana,
  Apple,
}
@Zamiell Zamiell added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Apr 25, 2022
@JoshuaKGoldberg
Copy link
Member
JoshuaKGoldberg commented Apr 25, 2022

Hmm, I like this idea, but it's a little tricky. Some projects are intentional about the order of their enums:

enum Direction {
   None, // 0
   Horizontal, // 1
   Vertical, // 2
   Both // 3
}

We could make sorting enums opt-in. It shouldn't flag enum values with initializers (None = 0). Marking as accepting PRs!

Tangentially, I'm finding the https://typescript-eslint.io/rules/member-ordering docs a bit confusing. Filed #4861 to clarify them.

In the meantime, eslint-plugin-typescript-sort-keys has a string-enum rule that does [edit: something similar to] what you're looking for.

@JoshuaKGoldberg JoshuaKGoldberg added enhancement: plugin rule option New rule option for an existing eslint-plugin rule accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Apr 25, 2022
@Zamiell
Copy link
Contributor Author
Zamiell commented Apr 25, 2022

Hey Josh, thanks for the quick reply.

In the meantime, eslint-plugin-typescript-sort-keys has a string-enum rule that does what you're looking for.

I need to sort numeric enums, like the Fruit enum included in my last example. Does the "string-enum" rule sort non-string-enums?

@JoshuaKGoldberg
Copy link
Member

Ah, sorry, you're right - that's missing in the plugin. 👎

@bradzacher
Copy link
Member
bradzacher commented Apr 25, 2022

Yeah I do understand the though process here - but it can be really dangerous to do this for implicitly valued enums because TS does not restrict comparisons against enums.

This means in your code you might have a comparison between the enum and a number (which TS fully understands), and by sorting the enum you would break your code.

enum Foo {
    A,
    B,
}

function test(arg: Foo) {
    if (arg === 1) {
        arg
        // ^? (parameter) arg: Foo.B
    }
}

play

This might be really difficult to debug and understand as it would break things at a distance, and potentially without any type errors.

I think if we do this, we should only implement it for enums with explicit values.

@JoshuaKGoldberg
Copy link
Member

Is there a common use case for comparisons like arg === 1, though? I can't remember the last time I saw enum comparisons that didn't go through the enum name like Foo.B. Maybe we should add a lint rule against it?

If we add a strong caveat to the docs, I would think it preferable that users take on the risk of fixing on their own. If the rule ever receives a suggestion autofix, this would be a case where we wouldn't want to emit one.

@bradzacher
Copy link
Member

Is there a common use case for comparisons like arg === 1, though? I can't remember the last time I saw enum comparisons that didn't go through the enum name like Foo.B.

TS doesn't block it, and even understands it - meaning it's 100% on the dev to make sure the code doesn't sneak into the codebase.
We had this problem all the time with our codebase and comparisons against AST_NODE_TYPES and raw strings.

There is a whole thing around number-typed enums and flags and how damn unsafe they are.. There is also some degree of "yeah it kind of makes sense" for them because you might do like flag = Foo.A ^ 3; value & flag === 0 or something - where comparisons and raw numbers make some semblance of sense.

There's also a whole discussion about public APIs. If you export an enum and someone consumes your library from raw JS - technically they're well within their rights to pass a number instead of explicitly using the enum. So by reordering the enum you would introduce a breaking change in your API.

Maybe we should add a lint rule against it?

#308

We also have the prefer-enum-initializers lint rule which warns against implicit enum values.

@Zamiell
Copy link
Contributor Author
Zamiell commented Apr 25, 2022

#308 is probably the best proposition for a linting rule that I have ever seen. Once the rule exists, it seems like it should be immediately turned on for virtually any TypeScript codebase.

The prefer-enum-initializers rule that Brad mentions only seems to be useful in a world where the rule from #308 does not exist and/or is not turned on.

Thus, in a world where the rule from #308 does exist and is included in the recommended config, I propose that the correct behavior for the member-ordering rule should be to function in the way that Josh describes here.

Should I try to submit a PR for #308 first? It seems like it would be fairly easy.

@bradzacher
Copy link
Member

The prefer-enum-initializers rule that Brad mentions only seems to be useful in a world where the rule from #308 does not exist and/or is not turned on.

It's useful for the above reasons I mentioned - to ensure the public API is clear and consistent.
Without initialisers - changing the order of your enums will change their values, which is a breaking change to your public API.
As an example - TS has a few enums which have implicit values, and they can easily cause problems with our project. Eg; if the user has two versions of TS installed then it's possible that we can parse with one version and lint with another. This means that the TS AST's "node kind" property has a set of values which align with a different enum. So when we check ts.SyntaxKind.Foo we might accidentally match ts.SyntaxKind.Bar, and then everything breaks in weird ways.

Implicit initialisers can be pretty unsafe and dangerous unless their use is very, very targeted and limited. So the lint rule does have use to enforce a better standard in your codebase!

Should I try to submit a PR for #308 first? It seems like it would be fairly easy.

You can certainly try if you'd like! It's not as easy as you might think just because you will need to cover all of the "comparison" locations, all of the "assignment" locations, and also correctly gather the types from those locations. It's not super trivial to do this!!!
The no-unsafe-* rules have examples for a lot of locations that would help get started!

@Zamiell
Copy link
Contributor Author
Zamiell commented Apr 25, 2022

Without initialisers - changing the order of your enums will change their values, which is a breaking change to your public API.

Ok. I understand the problem now. Authors of TypeScript libraries need to consider JavaScript consumers, so it is safer to not have the warning turned on by default, since it could cause people to accidentally break their API.

I'm not sure how to restructure the PR to accomplish that though, so I think I'll need a little help with that part.

@Zamiell
Copy link
Contributor Author
Zamiell commented Jul 23, 2022

If anyone needs enum sorting, I'm hosting a modified version of the rule here: https://github.com/IsaacScript/isaacscript/blob/main/packages/eslint-plugin-isaacscript/docs/rules/member-ordering.md

(You can consume it from NPM via the eslint-plugin-isaacscript package.)

All it does is implement this PR: #4862

@rubiesonthesky
Copy link
Contributor

I think this is same case as another feature request for sorting enums, #4008 (comment) . There is another eslint plugin that you can use if needed ( https://github.com/azat-io/eslint-plugin-perfectionist )

@JoshuaKGoldberg
Copy link
Member

I published prettier-plugin-sort-members as #2296 (comment) suggests 🚀 I appreciate if you try it out.

Edit: This supports only TypeScript file as of now. Edit: Also support JavaScript. Not yet for babel-ts parser.

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Mar 28, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2024
@bradzacher bradzacher added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Apr 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

4 participants
0