feat(js_analyze): add `lint/noEnum` #3653

nickfla1 · 2024-08-13T16:07:36Z

Summary

Added the noEnum TypeScript rule to disallow of enums altogether.

Test Plan

Run cargo test.

`quick_test` changes to test this rule:

diff --git a/crates/biome_js_analyze/src/lib.rs b/crates/biome_js_analyze/src/lib.rs
index 5883fe6caa..029fa40916 100644
--- a/crates/biome_js_analyze/src/lib.rs
+++ b/crates/biome_js_analyze/src/lib.rs
@@ -180,7 +180,7 @@ mod tests {
     use crate::react::hooks::StableHookResult;
     use crate::{analyze, AnalysisFilter, ControlFlow};
 
-    #[ignore]
+    // #[ignore]
     #[test]
     fn quick_test() {
         fn markup_to_string(markup: Markup) -> String {
@@ -192,7 +192,7 @@ mod tests {
             String::from_utf8(buffer).unwrap()
         }
 
-        const SOURCE: &str = r#"import buffer from "buffer"; "#;
+        const SOURCE: &str = r#"enum Foo { Bar = 'bar' }; "#;
 
         let parsed = parse(SOURCE, JsFileSource::tsx(), JsParserOptions::default());
 
@@ -204,7 +204,7 @@ mod tests {
             dependencies_index: Some(1),
             stable_result: StableHookResult::None,
         };
-        let rule_filter = RuleFilter::Rule("style", "useNodejsImportProtocol");
+        let rule_filter = RuleFilter::Rule("nursery", "noEnum");
 
         options.configuration.rules.push_rule(
             RuleKey::new("nursery", "useHookAtTopLevel"),

nickfla1 · 2024-08-13T16:08:50Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

I used noConsole as inspiration to write diagnostic and documentation, but I understand it can feel a little lacking.

I'm open to suggestions!

I believe it would be helpful to suggest an alternative in this case. For example, recommending a TypeScript object as a replacement for enums

chansuke

Thank you for your contribution and a good start! I've added a few comments. :)

chansuke · 2024-08-13T17:29:09Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

+    ///     BAR = 'bar'
+    /// }
+    /// ```
+    ///


You can add a valid case like below?

/// ### Valid /// /// ```js /// const Foo = { /// BAR: 'bar', /// }; /// ```

chansuke · 2024-08-13T17:33:45Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

I believe it would be helpful to suggest an alternative in this case. For example, recommending a TypeScript object as a replacement for enums

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

codspeed-hq · 2024-08-13T18:32:47Z

CodSpeed Performance Report

Merging #3653 will not alter performance

_{Comparing nickfla1:feat/no-enum (8c1f632) with main (b16def6)}

Summary

✅ 101 untouched benchmarks

ematipico · 2024-08-14T07:16:39Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

+                    "Don't use "<Emphasis>"enum"</Emphasis>
+                },
+            )
+            .note(markup! {
+                "Prefer using JavaScript objects or TypeScript unions over enums."
+            }),


The diagnostics are good, however we miss the explanation of the error. Here, we need to explain why a user shouldn't use enums.

Please refer to our rule pillars: https://biomejs.dev/linter/#rule-pillars

Hey @nickfla1, I noticed 5694370 (#3653). The reason should be added to the diagnostic too :)

Conaclos

Some questions / suggestions:

We already have a rule that bans const enum. I tis a recommended rule.
Should we restrict noEnum to regular enums?
This could avoid double diagnostics and more flexibility: a user could allow const enum and disallow enum.
I think we could allow declare enum and enums in TS declaration files (See rules such as noEvolvingTypes that checks if a file is a declaration/definition file) and in any ambient context (we have a is_ambient method that can be applied on a TsEnumdeclaration node)

nickfla1 · 2024-08-19T07:52:45Z

Some questions / suggestions:

We already have a rule that bans const enum. I tis a recommended rule.
Should we restrict noEnum to regular enums?
This could avoid double diagnostics and more flexibility: a user could allow const enum and disallow enum.

I think we could allow declare enum and enums in TS declaration files (See rules such as noEvolvingTypes that checks if a file is a declaration/definition file) and in any ambient context (we have a is_ambient method that can be applied on a TsEnumdeclaration node)

I think I agree on everything you've said! I'll wait for confirmation from @ematipico before implementing it in the PR

ematipico · 2024-08-19T08:20:35Z

Sure go ahead

Conaclos · 2024-08-19T13:53:00Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

+        let source_type = ctx.source_type::<JsFileSource>().language();
+        let is_declaration = source_type.is_definition_file();
+
+        if is_declaration {
+            return None
+        }


I think we should check it before checking for const/ambient enums.

Conaclos · 2024-08-19T13:53:18Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

+        let is_ambient = enum_decl.is_ambient();
+
+        if is_const_decl || is_ambient {


Suggested change

let is_ambient = enum_decl.is_ambient();

if is_const_decl || is_ambient {

if is_const_decl || enum_decl.is_ambient() {

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

Conaclos · 2024-08-19T13:56:40Z

crates/biome_js_analyze/tests/specs/nursery/noEnum/invalid.ts.snap

@@ -23,7 +23,7 @@ invalid.ts:1:1 lint/nursery/noEnum ━━━━━━━━━━━━━━━
      │ ^
    4 │ 

-  i Prefer using JavaScript objects or TypeScript unions over enums.
+  i Prefer using JavaScript objects, TypeScript unions or const enums over plain enums.


const enum have some important caveat (cf noConstEnum, docs). Moreover, we recommend noConstEnum.
Thus, I think it is not a good idea to suggest using them here.

Suggested change

i Prefer using JavaScript objects, TypeScript unions or const enums over plain enums.

i Prefer using JavaScript objects or TypeScript unions over plain enums.

Conaclos

Looks good to me. I left my last suggestions :)

Conaclos · 2024-08-24T10:49:26Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

+            .note(markup! {
+                "Enums will get compiled into JavaScript code, which can increase bundle size and cause side-effects in the codebase."
+            }),


This is not always true because some bundlers such as ESBuild or TSC are able to inline some enums. Thus, this can even be the reverse: using TS enum can improve code size and performance.

I suggest removing this note.

Conaclos · 2024-08-24T10:50:40Z

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

+            .note(markup! {
+                "Prefer using JavaScript objects or TypeScript unions over plain enums."
+            })


I propose this rephrasing:

Suggested change

.note(markup! {

"Prefer using JavaScript objects or TypeScript unions over plain enums."

})

.note(markup! {

"Use JavaScript objects or TypeScript unions instead."

})

Also, to follow the order of our rule pillars, I could put this note after other notes.

crates/biome_js_analyze/src/lint/nursery/no_enum.rs

nickfla1 · 2024-08-27T12:14:06Z

I have updated the docs and notes accordingly.

I am wondering what's the best approach to solve the conflict on that file since it's autogenerated.

ematipico · 2024-08-27T14:09:36Z

I have updated the docs and notes accordingly.

I am wondering what's the best approach to solve the conflict on that file since it's autogenerated.

Merge from main and accept the changes. Then run just gen-lint and push the new changes.

chansuke

lgtm

Conaclos

This looks good to me :) Thanks for your contribution!

nickfla1 · 2024-08-28T07:54:13Z

Happy to help! Thank you for your support and suggestions :)

feat(biome_js_analyze): noEnum

6732828

github-actions bot added A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Aug 13, 2024

nickfla1 commented Aug 13, 2024

View reviewed changes

feat(biome_js_analyze): update snapshot

ee3d236

chansuke requested changes Aug 13, 2024

View reviewed changes

docs(biome_js_analyze): added valid case and updated diagnostic message

dd186bc

ematipico reviewed Aug 14, 2024

View reviewed changes

nickfla1 added 2 commits August 14, 2024 12:32

docs(biome_js_analyze): add reasons to avoid enums

5694370

docs(biome_js_analyze): add reasons to avoid enums to diagnostics

e737cd5

Conaclos changed the title ~~feat(biome_js_analyze): noEnum~~ feat(js_analyze): add lint/noEnum Aug 18, 2024

Conaclos reviewed Aug 18, 2024

View reviewed changes

nickfla1 added 2 commits August 19, 2024 13:46

feat(biome_js_analyze): allow const enums and declarations

52089b6

feat(biome_js_analyze): allow ambient context

b345240

Conaclos reviewed Aug 19, 2024

View reviewed changes

crates/biome_js_analyze/src/lint/nursery/no_enum.rs Show resolved Hide resolved

Conaclos reviewed Aug 19, 2024

View reviewed changes

nickfla1 added 5 commits August 19, 2024 16:24

feat(biome_js_analyze): invert run login and update docs

986d9a7

fix(biome_js_analyze): missing semi

7a1553c

fix(biome_js_analyze): missing assign

538e8fe

Merge branch 'main' into feat/no-enum

f8cea14

fix(biome_js_analyze): missing commas

df676b5

Conaclos reviewed Aug 24, 2024

View reviewed changes

feat(biome_js_analyze): update doc

f1ffda5

nickfla1 added 2 commits August 27, 2024 16:27

Merge branch 'main' into feat/no-enum

66044d7

chore(biome_js_analyze): update codegen file

8c1f632

ematipico approved these changes Aug 27, 2024

View reviewed changes

ematipico requested review from chansuke and Conaclos August 27, 2024 15:35

chansuke approved these changes Aug 27, 2024

View reviewed changes

Conaclos approved these changes Aug 27, 2024

View reviewed changes

Conaclos merged commit 2d590ea into biomejs:main Aug 27, 2024
12 checks passed

nickfla1 deleted the feat/no-enum branch August 28, 2024 07:54

Conaclos added the A-Changelog Area: changelog label Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(js_analyze): add `lint/noEnum` #3653

feat(js_analyze): add `lint/noEnum` #3653

nickfla1 commented Aug 13, 2024 •

edited

Loading

nickfla1 Aug 13, 2024

chansuke Aug 13, 2024

chansuke left a comment

chansuke Aug 13, 2024

chansuke Aug 13, 2024

codspeed-hq bot commented Aug 13, 2024 •

edited

Loading

ematipico Aug 14, 2024

ematipico Aug 15, 2024

Conaclos left a comment •

edited

Loading

nickfla1 commented Aug 19, 2024

ematipico commented Aug 19, 2024

Conaclos Aug 19, 2024

Conaclos Aug 19, 2024

Conaclos Aug 19, 2024

Conaclos left a comment

Conaclos Aug 24, 2024

Conaclos Aug 24, 2024

nickfla1 commented Aug 27, 2024

ematipico commented Aug 27, 2024

chansuke left a comment

Conaclos left a comment •

edited

Loading

nickfla1 commented Aug 28, 2024

		let is_ambient = enum_decl.is_ambient();

		if is_const_decl \|\| is_ambient {

	i Prefer using JavaScript objects, TypeScript unions or const enums over plain enums.
	i Prefer using JavaScript objects or TypeScript unions over plain enums.

feat(js_analyze): add lint/noEnum #3653

feat(js_analyze): add lint/noEnum #3653

Conversation

nickfla1 commented Aug 13, 2024 • edited Loading

Summary

Test Plan

quick_test changes to test this rule:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chansuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codspeed-hq bot commented Aug 13, 2024 • edited Loading

Merging #3653 will not alter performance

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conaclos left a comment • edited Loading

Choose a reason for hiding this comment

nickfla1 commented Aug 19, 2024

ematipico commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conaclos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickfla1 commented Aug 27, 2024

ematipico commented Aug 27, 2024

chansuke left a comment

Choose a reason for hiding this comment

Conaclos left a comment • edited Loading

Choose a reason for hiding this comment

nickfla1 commented Aug 28, 2024

feat(js_analyze): add `lint/noEnum` #3653

feat(js_analyze): add `lint/noEnum` #3653

nickfla1 commented Aug 13, 2024 •

edited

Loading

`quick_test` changes to test this rule:

codspeed-hq bot commented Aug 13, 2024 •

edited

Loading

Conaclos left a comment •

edited

Loading

Conaclos left a comment •

edited

Loading