Skip to content

Disable @typescript-eslint/no-unsafe-enum-comparison rule#387

Merged
Mrtenz merged 2 commits into
mainfrom
mrtenz/disable-@typescript-eslint/no-unsafe-enum-comparison
Apr 17, 2025
Merged

Disable @typescript-eslint/no-unsafe-enum-comparison rule#387
Mrtenz merged 2 commits into
mainfrom
mrtenz/disable-@typescript-eslint/no-unsafe-enum-comparison

Conversation

@Mrtenz

@Mrtenz Mrtenz commented Feb 19, 2025

Copy link
Copy Markdown
Member

This disables the @typescript-eslint/no-unsafe-enum-comparison rule, which disallows comparing enums to string or number values. For example, the following is not allowed when this rule is enabled:

enum Foo {
  Bar = 'Bar',
  Baz = 'Baz',
}

const myVariable: string = "Bar";
if (myVariable == Foo.Bar) {
  // ...
}

We use this pattern in multiple places, so I think this rule is too strict and we should disable it.

@Mrtenz Mrtenz marked this pull request as ready for review February 19, 2025 13:13
@Mrtenz Mrtenz requested review from a team as code owners February 19, 2025 13:13
@Gudahtt

Gudahtt commented Feb 19, 2025

Copy link
Copy Markdown
Member

It's unclear why we'd use a string if we have an enum representing that string in-scope already. Could you elaborate on why this is "too strict"?

@mcmire

mcmire commented Feb 28, 2025

Copy link
Copy Markdown
Contributor

Ha, I was just about to submit a PR to disable this rule too.

Here is what I was going to write in the commit message:


The authors of the @typescript-eslint/no-unsafe-enum-comparison rule seem to think that enums should always be treated as a set of opaque labels rather than an object container of known values or mere shortcuts for string values. This means that the following is disallowed:

// The lint error occurs here (note `namespace` is a `string`, and we do not know what it is at compile time)
switch (namespace) {
  case KnownCaipNamespace.Wallet:
    if (reference === KnownCaipNamespace.Eip155) {
      return isSupportedEip155Account();
    }
    return isSupportedNonEvmAccount();
  case KnownCaipNamespace.Eip155:
    return isSupportedEip155Account();
  default:
    return isSupportedNonEvmAccount();
}

However, this pattern is very common across our code, so at the moment we get all kinds of errors.

While there is past discussion on this issue which explains that this is an unsafe operation because there's no guarantee that the string has any overlap with the enum, whether or not the string overlaps is the very thing we are trying to test. And while it is true that enum values could technically be anything, we are making what I think is a reasonable assumption that enum values are well-named, so KnownCaipNamespace.Eip155 equates to something like "eip155" and not "asdfadfdafasdlf".

Since we're slowly moving away from enums, I think we should disable this rule.

@Gudahtt

Gudahtt commented Apr 17, 2025

Copy link
Copy Markdown
Member

Ah I see, fair enough.

I feel kinda on the fence about this, as I do somewhat agree with the authors of the rule that enums are maybe best used just for opaque values. It's commonplace to use them with real values as well of course, even across languages, but it does feel like a distinct case from the "original purpose" or best use of enums. For cases where the value has meaning, we can use constants instead.

But it's a moot point if we're moving away from enums anyway, and this pattern at least is more of a style thing than a genuine cause of bugs. Agreed we should disable it.

@Gudahtt Gudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants