Skip to content

Rule proposal: no-enum / ban-enum / no-use-enum #561

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
kachkaev opened this issue May 27, 2019 · 9 comments · Fixed by #2117
Closed

Rule proposal: no-enum / ban-enum / no-use-enum #561

kachkaev opened this issue May 27, 2019 · 9 comments · Fixed by #2117
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@kachkaev
Copy link

kachkaev commented May 27, 2019

After switching to TypeScript, our team's monorepo ended up with quite a few enums in various parts of the code. Initially, referring to enums felt better than exporting named constants as we did in the good old JavaScript times (export const STATUS_READY = "ready"). However, we noticed a few issues with enums afterwards and decided to switch to TypeScript discriminated unions instead. The most irritating issue with enums for us was that their imports could cause cyclic module dependencies, thus magically resulting undefined in unexpected places, e.g. when running Jest tests.

We have removed all enums from the codebase, but there is still a chance that someone can introduce a new enum via a PR and this will be overlooked. I'm wondering if @typescript-eslint/eslint-plugin could have a rule named like no-enum / ban-enum / no-use-enum to prevent this from happening.

Example of incorrect code for this rule:

enum Direction {
  Up,
  Down,
  Left,
  Right,
}

Example of correct code for this rule:

type Direction = "up" | "down" | "left" | "right"

All the rule would have to do is searching for the enum token – that does not seem too complex. WDYT folks?

Related: #280 (see comment by @leoyli with a similar request)

@kachkaev kachkaev added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels May 27, 2019
@leoyli
Copy link

leoyli commented May 27, 2019

I think this also kind of useful, and perhaps it can be auto-fixed in some cases. 👍

@bradzacher
Copy link
Member

we don't like to implement rules that just ban a language feature, because we've found that different people like different things, and unless it's preposterous that you'd use the inverse (like no-ts-ignore), we would want to support the opposite as well.

Happy for something like this to be implemented, but we may want to review it in conjunction with the other enum rules with PRs (#290, #315)

@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed triage Waiting for team members to take a look labels May 28, 2019
@leoyli
Copy link

leoyli commented May 28, 2019

True, I feel there are many similar rules flooded, and would like to have 1 entry point to regulate how enum could be used.

We are using TS as just a pure type checker, so any non-type-system-related syntax/operators will out of our interests. Enum surely have its own syntactical advantages, but because of Babel won't be able to support it properly, then this could be 1 exception here. This rule is helpful to avoid surprise to see why "my const enum code doesn't work".

@platinumazure
Copy link
Contributor

Will the core no-restricted-syntax work here? I think it should since the enums are language constructs that are traversed-- you would just need to know the proper selector for reporting on them when they are used. (I don't know what that proper selector is, sorry.)

@bradzacher
Copy link
Member

bradzacher commented May 28, 2019

I believe that would work @platinumazure

{
  "rules": {
    "no-restricted-syntax": [
      "error",
      {
        "selector": "TSEnumDeclaration",
        "message": "Don't declare enums"
      }
    ]
  }
}

@leoyli
Copy link

leoyli commented May 29, 2019

Tested, the above is the one I am looking for. But I think this rule might be useful if ESLint-TypeScript would ever provide the recommended default setup for Babel users. 🤔

@kimamula
Copy link

kimamula commented Mar 1, 2020

It may be worth noting that you can also disallow either const or non-const enums selectively with no-restricted-syntax rule.

Disallow const enums:

{
  "rules": {
    "no-restricted-syntax": [
      "error",
      {
        "selector": "TSEnumDeclaration[const=true]",
        "message": "Don't declare const enums"
      }
    ]
  }
}

Disallow non-const enums:

{
  "rules": {
    "no-restricted-syntax": [
      "error",
      {
        "selector": "TSEnumDeclaration:not([const=true])",
        "message": "Don't declare non-const enums"
      }
    ]
  }
}

@halfnibble
Copy link

Is there any way to selectively do this so that const enums are allowed for internal package use, but cannot be exported for external consumption?

@bradzacher
Copy link
Member

It's pretty hard / near impossible to detect this.
What does it mean to not be exported? It's a project dependent definition.
Without bundling, every file is technically available to import by a consumer.
With bundling, what is exported depends on how your bundler is configured - i.e. what entry points you define.

A rule like that would have to understand the above, and be able to scan every export from the "external api", and crawl every single type to find any const enums.

I.e. very hard to do, very niche, and outside of the scope of this plugin.

@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 New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants