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.

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

Rule proposal: Const enum #280

Closed
sindresorhus opened this issue Feb 14, 2019 · 19 comments · Fixed by #2117
Closed

Rule proposal: Const enum #280

sindresorhus opened this issue Feb 14, 2019 · 19 comments · Fixed by #2117
Labels
enhancement: new plugin rule package: eslint-plugin

Comments

@sindresorhus
Copy link

sindresorhus commented Feb 14, 2019

The user would be able to configure this rule to either enforce always using const enum or always disallow it.

Relevant TSLint issue: palantir/tslint#1798

@sindresorhus sindresorhus added package: eslint-plugin triage labels Feb 14, 2019
@bradzacher bradzacher added enhancement: new plugin rule and removed triage labels Feb 14, 2019
@j-f1
Copy link
Contributor

j-f1 commented Feb 14, 2019

This would be valuable since Babel doesn’t support const enum.

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 14, 2019

My take on enums:

in a lot of cases const enums can be replaced with literal types. actual enums are useful only if they take advantage of declaration merging otherwise they should be const enum which can be replaced with literal types.

@bradzacher
Copy link
Member

bradzacher commented Feb 14, 2019

enums are usually nicer than literals for two reasons:

  1. You can use the TS language service to refactor rename them across your workspace.
  2. If you're not basing the enum of an external API, you can make them opaque (i.e. don't provide a value) - meaning people can't accidentally use literals in comparisons, etc
    • using literals is really hard to refactor, and can cause bugs if you're not careful

Example:

enum StrFoo {
    a = 'a'
}
const y: StrFoo = StrFoo.a;
if (y === 'a') {  // is compile-time safe, but hard to refactor
    
}

enum NumFoo {
    a = 1
}
const z: NumFoo = NumFoo.a;
if (z === 1) { // is compile-time safe, but hard to refactor
    
}

enum Foo {
    a
}
const x: Foo = Foo.a;
if (x === 'a') { // is NOT compile-time safe
    
}

repl

If you refactor enum names or adjust the values, then you can very easily create subtle bugs that are type safe

 enum NumFoo {
-   a = 1
+   a = 0
+   b = 1
 }
 const y: NumFoo = NumFoo.a;
 if (y === 1) {  // this check is compile-time correct, but the logic is now broken because we've changed the enum values.
     
 }

There's a big reason that the flow team is scrambling to add enums to their language - because using string literals and string unions is really, really clunky and causes you to have to write a lot of unnecessary code.

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 14, 2019

Never considered refactoring. That's a very good point! 👏🏽

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 16, 2019

I'm working on a rule for this. I call it enum-style with following options:

Default config:

{
    "enum-style": ["error", "no-const"]
}
  • no-const Do not allow const enum
  • no-enum Do not allow any type of enum
  • const Only const enum is allowed

@mysticatea
Copy link
Member

mysticatea commented Feb 16, 2019

The rule name sounds good.
But I'd like to suggest the option names are not negated. The option name should show the style people use, rather than it implicitly shows style by disallowing the other side.

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 16, 2019

How about this:

  • enum: Only non-const enum is allowed
    • This option name doesn't feel clear to me. Maybe ban-const?
  • ban Do not allow any type of enum
  • const Only const enum is allowed

@mysticatea
Copy link
Member

mysticatea commented Feb 17, 2019

How about an option object:

{
  "const": "always" | "never" | "any",
  // "memberType": "number" | "string" | "any"
}

I'm not sure if this rule disallows enum syntax itself. Do other options disallow the union type of literal types? If no, it's not symmetry.

@mysticatea
Copy link
Member

mysticatea commented Feb 17, 2019

I'm leaning towards focus on only const.

{
    "enum-const-style": ["error", "always" | "never"],
    // "enum-member-style":  ["error", "number" | "string"],
    // "enum-syntax-style":  ["error", "enum" | "literal-union"],
}

This should be the intention of this issue.

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 18, 2019

bringing const to name makes the "ban" option a bit weird. I agree that other options I mentioned in the PR belong to their own rules.

@mysticatea
Copy link
Member

mysticatea commented Feb 18, 2019

bringing const to name makes the "ban" option a bit weird.

I agree and we should remove ban from the rule. My comment "focus on only const" means removing ban option. It was not related to this issue originally. You can use no-restricted-syntax rule to disallow arbitrary syntax.

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 18, 2019

Update my PR to use "enum-const-style" name.

Can you please explain what "enum-syntax-style" will do?

@mysticatea
Copy link
Member

mysticatea commented Feb 18, 2019

Sure. I have thought enum-syntax-style rule to enforce people use either enum syntax or the union type of literal types. "enum" option will enforce to use enum syntax by disallowing the union type of literal types. "literal-union" option is the other side. (I thought such a symmetry is important in stylistic rules. If options don't have symmetry, probably the rule tries to solve two or more matters, and we should consider to split it.)

But we should open an issue to discuss it because I'm not sure if the core team accepts it.

@phaux
Copy link
Contributor

phaux commented Apr 18, 2019

I'm pretty sure you can refactor variant of union of strings at least in VSCode

@bradzacher bradzacher added the has pr label Apr 25, 2019
@leoyli
Copy link

leoyli commented May 21, 2019

Was attempting to open an issue for ban-enum or no-use-enum, and get known I can use no-restricted-syntax to ban enum completely.

I personally don't like enum since (1) it still an ECMAScript reserved word; (2) it don't looks new-user friendly, a bit daunting to me; (3) prefer idiomatic JavaScript (the same reason for me to favor type over interface).

After parse though this issue, I'm wondering if the proposed rules here would allow banning enum completely?!

@bradzacher bradzacher removed the has pr label Apr 13, 2020
@benny-medflyt
Copy link

benny-medflyt commented May 27, 2020

This issue should be closed as a duplicate of #561

#561 (comment) also explains how to solve this using no-restricted-syntax

@bradzacher
Copy link
Member

bradzacher commented May 27, 2020

They're very different use cases.

That one is about banning enums altogether.

This one is about allowing enums, but configuring to enforce always using a const enum, or never using a const enum.

@benny-medflyt
Copy link

benny-medflyt commented May 27, 2020

As I mentioned, the comment in issue #561 (comment) explains exactly how to configure eslint to force always use const enum, or to force always use non-const enum

@bradzacher
Copy link
Member

bradzacher commented May 27, 2020

Sorry, I was on mobile so I missed that.

I'll close both this and the other issue with an FAQ article with examples on using no-restricted-syntax

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule package: eslint-plugin
Projects
None yet
8 participants