From fe38ca124e0a932223ea97f5b3e43b314edb4056 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 23 May 2022 15:08:57 -0400 Subject: [PATCH 1/5] docs: overhauled no-extraneous-class rule docs --- .../docs/rules/no-extraneous-class.md | 144 +++++++++++++++--- 1 file changed, 120 insertions(+), 24 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-extraneous-class.md b/packages/eslint-plugin/docs/rules/no-extraneous-class.md index eb229ea655aa..a5f4e8644d53 100644 --- a/packages/eslint-plugin/docs/rules/no-extraneous-class.md +++ b/packages/eslint-plugin/docs/rules/no-extraneous-class.md @@ -2,14 +2,21 @@ Forbids the use of classes as namespaces. -This rule warns when a class is accidentally used as a namespace. +This rule warns when a class has no non-static members, such as for a class used exclusively as a static namespace. ## Rule Details -From TSLint’s docs: +Users who come from a [OOP](https://en.wikipedia.org/wiki/Object-oriented_programming) paradigm may wrap their utility functions in an extra class, instead of putting them at the top level of an ECMAScript module. +Doing so is generally unnecessary in JavaScript and TypeScript projects. -> Users who come from a Java-style OO language may wrap their utility functions in an extra class, -> instead of putting them at the top level. +- Wrapper classes add extra runtime bloat and cognitive complexity to code without adding any structural improvements + - Whatever would be put on them, such as utility functions, are already organized by virtue of being in a module. + - As an alternative, you can always `import * as ...` the module to get all of them in a single object. +- IDEs can't provide as good autocompletions when you start typing the names of the helpers, since they're on a class instead of freely available to import +- It's more difficult to statically analyze code for unused variables, etc. when they're all on the class (see: [Finding dead code (and dead types) in TypeScript](https://effectivetypescript.com/2020/10/20/tsprune)). + +This rule also flags classes that have only a constructor and no fields. +Those classes can generally be replaced with a standalone function. Examples of code for this rule: @@ -18,18 +25,16 @@ Examples of code for this rule: ### ❌ Incorrect ```ts -class EmptyClass {} +class StaticConstants { + static readonly version = 42; -class ConstructorOnly { - constructor() { - foo(); + static isProduction() { + return process.env.NODE_ENV === 'production'; } } -// Use an object instead: -class StaticOnly { - static version = 42; - static hello() { +class HelloWorldLogger { + constructor() { console.log('Hello, world!'); } } @@ -38,18 +43,15 @@ class StaticOnly { ### ✅ Correct ```ts -class EmptyClass extends SuperClass {} +export const version = 42; -class ParameterProperties { - constructor(public name: string) {} +export function isProduction() { + return process.env.NODE_ENV === 'production'; } -const StaticOnly = { - version: 42, - hello() { - console.log('Hello, world!'); - }, -}; +function logHelloWorld() { + console.log('Hello, world!'); +} ``` ## Options @@ -76,14 +78,108 @@ const defaultOptions: Options = { }; ``` +This rule normally bans classes that are empty (have no constructor or fields). +The rule's options each add an exemption for a specific type of class. + +### `allowConstructorOnly` + +`allowConstructorOnly` adds an exemption for classes that have only a constructor and no fields. + + + +#### ❌ Incorrect + +```ts +class NoFields {} +``` + +#### ✅ Correct + +```ts +class NoFields { + constructor() { + console.log('Hello, world!'); + } +} +``` + +### `allowEmpty` + +The `allowEmpty` option adds an exemption for classes that are entirely empty. + + + +#### ❌ Incorrect + +```ts +class NoFields { + constructor() { + console.log('Hello, world!'); + } +} +``` + +#### ✅ Correct + +```ts +class NoFields {} +``` + +### `allowStaticOnly` + +The `allowStaticOnly` option adds an exemption for classes that only contain static members. + +:::caution +We strongly recommend against the `allowStaticOnly` exemption. +It works against this rule's primary purpose of discouraging classes used only for static members. +::: + + + +#### ❌ Incorrect + +```ts +class EmptyClass {} +``` + +#### ✅ Correct + +```ts +class NotEmptyClass { + static version = 42; +} +``` + +### `allowWithDecorator` + +The `allowWithDecorator` option adds an exemption for classes that contain a member decorated with a `@` decorator. + + + +#### ❌ Incorrect + +```ts +class Constants { + static readonly version = 42; +} +``` + +#### ✅ Correct + +```ts +class Constants { + @logOnRead() + static readonly version = 42; +} +``` + ## When Not To Use It -You can disable this rule if you don’t have anyone who would make these kinds of mistakes on your -team or if you use classes as namespaces. +You can disable this rule if you are unable -or unwilling- to switch off using classes as namespaces. ## Related To -[`no-unnecessary-class`](https://palantir.github.io/tslint/rules/no-unnecessary-class/) from TSLint +[`no-unnecessary-class`](https://palantir.github.io/tslint/rules/no-unnecessary-class) from TSLint ## Attributes From 9848cabe6d4d03a1b84df7a427e8201e33901482 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 23 May 2022 15:36:07 -0400 Subject: [PATCH 2/5] fix: cspell --- packages/eslint-plugin/docs/rules/no-extraneous-class.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/no-extraneous-class.md b/packages/eslint-plugin/docs/rules/no-extraneous-class.md index a5f4e8644d53..01c6b56a918c 100644 --- a/packages/eslint-plugin/docs/rules/no-extraneous-class.md +++ b/packages/eslint-plugin/docs/rules/no-extraneous-class.md @@ -12,7 +12,7 @@ Doing so is generally unnecessary in JavaScript and TypeScript projects. - Wrapper classes add extra runtime bloat and cognitive complexity to code without adding any structural improvements - Whatever would be put on them, such as utility functions, are already organized by virtue of being in a module. - As an alternative, you can always `import * as ...` the module to get all of them in a single object. -- IDEs can't provide as good autocompletions when you start typing the names of the helpers, since they're on a class instead of freely available to import +- IDEs can't provide as good suggestions when you start typing the names of the helpers, since they're on a class instead of freely available to import - It's more difficult to statically analyze code for unused variables, etc. when they're all on the class (see: [Finding dead code (and dead types) in TypeScript](https://effectivetypescript.com/2020/10/20/tsprune)). This rule also flags classes that have only a constructor and no fields. From a917483d6506ef0fd6c7e185dfaccb822a2a28e0 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 31 May 2022 15:09:32 -0400 Subject: [PATCH 3/5] docs: respond to feedback; more code snippets --- .../docs/rules/no-extraneous-class.md | 140 +++++++++++++++++- 1 file changed, 138 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-extraneous-class.md b/packages/eslint-plugin/docs/rules/no-extraneous-class.md index 01c6b56a918c..b324f99b5530 100644 --- a/packages/eslint-plugin/docs/rules/no-extraneous-class.md +++ b/packages/eslint-plugin/docs/rules/no-extraneous-class.md @@ -9,10 +9,10 @@ This rule warns when a class has no non-static members, such as for a class used Users who come from a [OOP](https://en.wikipedia.org/wiki/Object-oriented_programming) paradigm may wrap their utility functions in an extra class, instead of putting them at the top level of an ECMAScript module. Doing so is generally unnecessary in JavaScript and TypeScript projects. -- Wrapper classes add extra runtime bloat and cognitive complexity to code without adding any structural improvements +- Wrapper classes add extra cognitive complexity to code without adding any structural improvements - Whatever would be put on them, such as utility functions, are already organized by virtue of being in a module. - As an alternative, you can always `import * as ...` the module to get all of them in a single object. -- IDEs can't provide as good suggestions when you start typing the names of the helpers, since they're on a class instead of freely available to import +- IDEs can't provide as good suggestions for static class or namespace imported properties when you start typing property names - It's more difficult to statically analyze code for unused variables, etc. when they're all on the class (see: [Finding dead code (and dead types) in TypeScript](https://effectivetypescript.com/2020/10/20/tsprune)). This rule also flags classes that have only a constructor and no fields. @@ -54,6 +54,142 @@ function logHelloWorld() { } ``` +## Alternatives + +### Individual Exports (Recommended) + +Instead of using a static utility class we recommend you individually export the utilities from your module. + + + +#### ❌ Incorrect + +```ts +export class Utilities { + static util1() { + return Utilities.util3(); + } + + static util2() { + /* ... */ + } + + static util3() { + /* ... */ + } +} +``` + +#### ✅ Correct + +```ts +export function util1() { + return util3(); +} + +export function util2() { + /* ... */ +} + +export function util3() { + /* ... */ +} +``` + +### Namespace Imports (Not Recommended) + +If you strongly prefer to have all constructs from a module available as properties of a single object, you can `import * as` the module. +This is known as a "namespace import". +Namespace imports are sometimes preferable because they keep all properties nested and don't need to be changed as you start or stop using various properties from the module. + +However, namespace imports are impacted by XYZ downsides: + +- They also don't play as well with tree shaking in modern bundlers +- They require a name prefix before each property's usage + + + +#### ❌ Incorrect + +```ts +// utilities.ts +export class Utilities { + static sayHello() { + console.log('Hello, world!'); + } +} + +// consumers.ts +import { Utilities } from './utilities'; + +utilities.sayHello(); +``` + +#### ⚠️ Namespace Imports + +```ts +// utilities.ts +export function sayHello() { + console.log('Hello, world!'); +} + +// consumers.ts +import * as utilities from './utilities'; + +sayHello(); +``` + +#### ✅ Standalone Imports + +```ts +// utilities.ts +export function sayHello() { + console.log('Hello, world!'); +} + +// consumers.ts +import { sayHello } from './utilities'; + +sayHello(); +``` + +### Notes on Mutating Variables + +One case you need to be careful of is exporting mutable variables. +While class properties can be mutated externally, exported variables are always constant. +This means that importers can only ever read the first value they are assigned and cannot write to the variables. + +Needing to write to an exported variable is very rare and is generally considered a code smell. +If you do need it you can accomplish it using getter and setter functions: + + + +#### ❌ Incorrect + +```ts +export class Utilities { + static mutableCount = 1; + + static incrementCount() { + Utilities.mutableCount += 1; + } +} +``` + +#### ✅ Correct + +```ts +let mutableCount = 1; + +export function getMutableCount() { + return mutableField; +} + +export function incrementCount() { + mutableField += 1; +} +``` + ## Options This rule accepts a single object option. From 210707a3a805d0b1200813e5fede79c3777dd426 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 2 Jun 2022 10:35:52 -0400 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Brad Zacher --- packages/eslint-plugin/docs/rules/no-extraneous-class.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-extraneous-class.md b/packages/eslint-plugin/docs/rules/no-extraneous-class.md index 777ea143b5b7..4b012b119452 100644 --- a/packages/eslint-plugin/docs/rules/no-extraneous-class.md +++ b/packages/eslint-plugin/docs/rules/no-extraneous-class.md @@ -122,7 +122,7 @@ export class Utilities { // consumers.ts import { Utilities } from './utilities'; -utilities.sayHello(); +Utilities.sayHello(); ``` #### ⚠️ Namespace Imports @@ -136,7 +136,7 @@ export function sayHello() { // consumers.ts import * as utilities from './utilities'; -sayHello(); +utilities.sayHello(); ``` #### ✅ Standalone Imports From 2d106beb15591a65e90e47f9b82f911f18eb5176 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 2 Jun 2022 10:36:43 -0400 Subject: [PATCH 5/5] Fix the XYZ --- packages/eslint-plugin/docs/rules/no-extraneous-class.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/docs/rules/no-extraneous-class.md b/packages/eslint-plugin/docs/rules/no-extraneous-class.md index 4b012b119452..aa221ce17fa0 100644 --- a/packages/eslint-plugin/docs/rules/no-extraneous-class.md +++ b/packages/eslint-plugin/docs/rules/no-extraneous-class.md @@ -102,7 +102,7 @@ If you strongly prefer to have all constructs from a module available as propert This is known as a "namespace import". Namespace imports are sometimes preferable because they keep all properties nested and don't need to be changed as you start or stop using various properties from the module. -However, namespace imports are impacted by XYZ downsides: +However, namespace imports are impacted by these downsides: - They also don't play as well with tree shaking in modern bundlers - They require a name prefix before each property's usage