fix(enums-eslint): Enum Rule for ESLint

[Patrick-Pimentel-Bitwarden]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-20454

📔 Objective

Added enums in the warnings for eslint via using a new eslint plugin. Set to warning right now and can be bumped later to error.

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.34%. Comparing base (624dafa) to head (af84567).

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #14650   +/-   ##
=======================================
  Coverage   36.34%   36.34%           
=======================================
  Files        3190     3190           
  Lines       92150    92150           
  Branches    16524    16524           
=======================================
  Hits        33491    33491           
  Misses      56268    56268           
  Partials     2391     2391           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Checkmarx One – Scan Summary & Details – 2be133be-15b8-45b8-9805-60a3c8b4181c

Great job, no security vulnerabilities found in this Pull Request

[Hinton]

Do we have clear plans on resolving this in the near future? If not I'd be more open to changing this to error and add ignore statements to all the existing.

Warnings tends to be ignored even with all the noise they produce, just look at the server project.

[coroiu]

I have the same concern as @Hinton: if all teams don't immediately start fixing these warnings we should add ignore ignore statements to all the existing code. Otherwise any new warning about usage of enum will completely drown in all of the other existing warnings

[Patrick-Pimentel-Bitwarden]

@Hinton @coroiu see decision by @withinfocus here. If both of you feel strongly about the proposed approach regarding erroring all of them and adding an ignore to the existing ones then let's have that conversation here in this pr.

[Hinton]

Yes I feel strongly about it since ignoring existing occurrences is the established standard. We have a long track history of ignoring warnings either explicitly or implicitly.

1. If we want to prevent new enums from being created the linting must be an error i.e. blocking. If we have 180 warnings we won't notice if it suddenly becomes 181.
2. Warnings are noisy, printing 180 warnings every time you run lint is not useful.
3. Warnings will not speed up a migration. Teams will already be aware enums should be migrated but it's not a trivial migration they won't be migrated until the team decides to prioritize the jira ticket.

[coroiu]

I agree that we've gone the ignore route for a while now and I think it has proven itself to work. It immediately changes how we write code so that we don't continue writing using deprecated syntax and it allows reviewers to instantly identify when someone might try to "sneak something in. An eslint-disable comment is very easy to spot and it allows anyone to question the addition even if they are unfamiliar with why it was needed in the first place.

If we make the rule a "warning" then we'll be waiting a long time for every single team to fix their existing instances (and maybe some new ones) and someone will have to actively follow up on it so we don't forget to flip the switch to error. We'll also be spamming the console but that's probably the smallest issue.

Here are some of the previous PRs where we've followed this convention (admittedly most are by you @Hinton so it probably shouldn't come as a surprise to anyone that you favor this approach 😄):

• Add eslint rule no-floating-promises #7789
• [deps] SM: Update typescript-eslint monorepo to v8 (major) #10601
• Add eslint-plugin-rxjs & rxjs-angular #3373
• [PM-16838] Forbid imports containing /src/ #12744

[Patrick-Pimentel-Bitwarden]

Oh cool! Thank you for filling me in on some of the traditional approaches we've taken and why. I'll update to conform and leave a note in the other thread for Matt to visit this conversation just in case he wants to know what's going on 👍

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud