Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
docs(eslint-plugin): [no-extraneous-class] overhaul rule docs#5059
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
bradzacher merged 10 commits intotypescript-eslint:mainfromJoshuaKGoldberg:no-extraneous-class-docsJun 4, 2022
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
10 commits Select commitHold shift + click to select a range
fe38ca1
docs: overhauled no-extraneous-class rule docs
JoshuaKGoldberg6e524be
Merge branch 'main' into no-extraneous-class-docs
JoshuaKGoldberg9848cab
fix: cspell
JoshuaKGoldbergc7b99a7
Merge branch 'no-extraneous-class-docs' of https://github.com/JoshuaK…
JoshuaKGoldberga917483
docs: respond to feedback; more code snippets
JoshuaKGoldberga592a3f
Merge branch 'main' into no-extraneous-class-docs
JoshuaKGoldberg7427886
Merge branch 'main'
JoshuaKGoldberg210707a
Apply suggestions from code review
JoshuaKGoldberg2d106be
Fix the XYZ
JoshuaKGoldberga1a5b17
Merge branch 'main' into no-extraneous-class-docs
JoshuaKGoldbergFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
276 changes: 254 additions & 22 deletionspackages/eslint-plugin/docs/rules/no-extraneous-class.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -2,14 +2,21 @@ | ||
Disallows classes used as namespaces. | ||
This rule warns when a classhas no non-static members, such as for a classusedexclusivelyas a static namespace. | ||
## Rule Details | ||
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 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. | ||
JoshuaKGoldberg marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
- 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)). | ||
JoshuaKGoldberg marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
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 | ||
bradzacher marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
```ts | ||
class StaticConstants { | ||
static readonly version = 42; | ||
static isProduction() { | ||
return process.env.NODE_ENV === 'production'; | ||
} | ||
} | ||
class HelloWorldLogger { | ||
constructor() { | ||
console.log('Hello, world!'); | ||
} | ||
} | ||
@@ -38,18 +43,151 @@ class StaticOnly { | ||
### ✅ Correct | ||
```ts | ||
export const version = 42; | ||
export function isProduction() { | ||
return process.env.NODE_ENV === 'production'; | ||
} | ||
function logHelloWorld() { | ||
console.log('Hello, world!'); | ||
} | ||
``` | ||
## Alternatives | ||
### Individual Exports (Recommended) | ||
Instead of using a static utility class we recommend you individually export the utilities from your module. | ||
<!--tabs--> | ||
#### ❌ 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 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 | ||
<!--tabs--> | ||
#### ❌ 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'; | ||
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: | ||
<!--tabs--> | ||
#### ❌ 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 | ||
@@ -76,10 +214,104 @@ 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. | ||
<!--tabs--> | ||
#### ❌ 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. | ||
<!--tabs--> | ||
#### ❌ 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. | ||
::: | ||
<!--tabs--> | ||
#### ❌ 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. | ||
<!--tabs--> | ||
#### ❌ 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 are unable -or unwilling- to switch off using classes as namespaces. | ||
## Related To | ||
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.