feat(eslint-plugin): [ban-types] rework default options (#848)

.markdownlint.json

@@ -58,7 +58,18 @@
   // MD032/blanks-around-lists - Lists should be surrounded by blank lines
   "MD032": false,
   // MD033/no-inline-html - Inline HTML
-  "MD033": { "allowed_elements": ["a", "img", "br", "sup", "h1", "p"] },
+  "MD033": {
+    "allowed_elements": [
+      "a",
+      "img",
+      "br",
+      "sup",
+      "h1",
+      "p",
+      "details",
+      "summary"
+    ]
+  },
   // MD034/no-bare-urls - Bare URL used
   "MD034": false,
   // MD035/hr-style - Horizontal rule style

packages/eslint-plugin/docs/rules/ban-types.md

@@ -1,100 +1,186 @@
 # Bans specific types from being used (`ban-types`)
 
-This rule bans specific types and can suggest alternatives. It does not ban the
-corresponding runtime objects from being used.
+Some builtin types have aliases, some types are considered dangerous or harmful.
+It's often a good idea to ban certain types to help with consistency and safety.
 
 ## Rule Details
 
-Examples of **incorrect** code for this rule `"String": "Use string instead"`
-
-```ts
-class Foo<F = String> extends Bar<String> implements Baz<String> {
-  constructor(foo: String) {}
-
-  exit(): Array<String> {
-    const foo: String = 1 as String;
-  }
-}
-```
-
-Examples of **correct** code for this rule `"String": "Use string instead"`
-
-```ts
-class Foo<F = string> extends Bar<string> implements Baz<string> {
-  constructor(foo: string) {}
-
-  exit(): Array<string> {
-    const foo: string = 1 as string;
-  }
-}
-```
+This rule bans specific types and can suggest alternatives.
+Note that it does not ban the corresponding runtime objects from being used.
 
 ## Options
 
-The banned type can either be a type name literal (`Foo`), a type name with generic parameter instantiations(s) (`Foo<Bar>`), or the empty object literal (`{}`).
-
-```CJSON
-{
-    "@typescript-eslint/ban-types": ["error", {
-        "types": {
-            // report usages of the type using the default error message
-            "Foo": null,
-
-            // add a custom message to help explain why not to use it
-            "Bar": "Don't use bar!",
-
-            // add a custom message, AND tell the plugin how to fix it
-            "String": {
-                "message": "Use string instead",
-                "fixWith": "string"
-            }
-
-            "{}": {
-              "message": "Use object instead",
-              "fixWith": "object"
-            }
-        }
-    }]
-}
+```ts
+type Options = {
+  types?: {
+    [typeName: string]:
+      | string
+      | {
+          message: string;
+          fixWith?: string;
+        };
+  };
+  extendDefaults?: boolean;
+};
 ```
 
-By default, this rule includes types which are likely to be mistakes, such as `String` and `Number`. If you don't want these enabled, set the `extendDefaults` option to `false`:
+The rule accepts a single object as options, with the following keys:
 
-```CJSON
-{
-    "@typescript-eslint/ban-types": ["error", {
-        "types": {
-            // add a custom message, AND tell the plugin how to fix it
-            "String": {
-                "message": "Use string instead",
-                "fixWith": "string"
-            }
-        },
-        "extendDefaults": false
-    }]
-}
-```
+- `types` - An object whose keys are the types you want to ban, and the values are error messages.
+  - The type can either be a type name literal (`Foo`), a type name with generic parameter instantiation(s) (`Foo<Bar>`), or the empty object literal (`{}`).
+  - The values can be a string, which is the error message to be reported,
+    or it can be an object with the following properties:
+    - `message: string` - the message to display when the type is matched.
+    - `fixWith?: string` - a string to replace the banned type with when the fixer is run. If this is omitted, no fix will be done.
+- `extendDefaults` - if you're specifying custom `types`, you can set this to `true` to extend the default `types` configuration.
+  - This is a convenience option to save you copying across the defaults when adding another type.
+  - If this is `false`, the rule will _only_ use the types defined in your configuration.
 
-### Example
+Example configuration:
 
-```json
+```jsonc
 {
   "@typescript-eslint/ban-types": [
     "error",
     {
       "types": {
-        "Array": null,
-        "Object": "Use {} instead",
+        // add a custom message to help explain why not to use it
+        "Foo": "Don't use Far because it is unsafe",
+
+        // add a custom message, AND tell the plugin how to fix it
         "String": {
           "message": "Use string instead",
           "fixWith": "string"
+        },
+
+        "{}": {
+          "message": "Use object instead",
+          "fixWith": "object"
         }
       }
     }
   ]
 }
 ```
 
+### Default Options
+
+The default options provide a set of "best practices", intended to provide safety and standardization in your codebase:
+
+- Don't use the upper-case primitive types, you should use the lower-case types for consistency.
+- Avoid the `Function` type, as it provides little safety for the following reasons:
+  - It provides no type safety when calling the value, which means it's easy to provide the wrong arguments.
+  - It accepts class declarations, which will fail when called, as they are called without the `new` keyword.
+- Avoid the `Object` and `{}` types, as they mean "any non-nullish value".
+  - This is a point of confusion for many developers, who think it means "any object type".
+- Avoid the `object` type, as it is currently hard to use due to not being able to assert that keys exist.
+  - See [microsoft/TypeScript#21732](https://github.com/microsoft/TypeScript/issues/21732).
+
+**_Important note:_** the default options suggest using `Record<string, unknown>`; this was a stylistic decision, as the built-in `Record` type is considered to look cleaner.
+
+<details>
+<summary>Default Options</summary>
+
+```ts
+const defaultTypes = {
+  String: {
+    message: 'Use string instead',
+    fixWith: 'string',
+  },
+  Boolean: {
+    message: 'Use boolean instead',
+    fixWith: 'boolean',
+  },
+  Number: {
+    message: 'Use number instead',
+    fixWith: 'number',
+  },
+  Symbol: {
+    message: 'Use symbol instead',
+    fixWith: 'symbol',
+  },
+
+  Function: {
+    message: [
+      'The `Function` type accepts any function-like value.',
+      'It provides no type safety when calling the function, which can be a common source of bugs.',
+      'It also accepts things like class declarations, which will throw at runtime as they will not be called with `new`.',
+      'If you are expecting the function to accept certain arguments, you should explicitly define the function shape.',
+    ].join('\n'),
+  },
+
+  // object typing
+  Object: {
+    message: [
+      'The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.',
+      '- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.',
+      '- If you want a type meaning "any value", you probably want `unknown` instead.',
+    ].join('\n'),
+  },
+  '{}': {
+    message: [
+      '`{}` actually means "any non-nullish value".',
+      '- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.',
+      '- If you want a type meaning "any value", you probably want `unknown` instead.',
+    ].join('\n'),
+  },
+  object: {
+    message: [
+      'The `object` type is currently hard to use ([see this issue](https://github.com/microsoft/TypeScript/issues/21732)).',
+      'Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys.',
+    ].join('\n'),
+  },
+};
+```
+
+</details>
+
+### Examples
+
+Examples of **incorrect** code with the default options:
+
+```ts
+// use lower-case primitives for consistency
+const str: String = 'foo';
+const bool: Boolean = true;
+const num: Number = 1;
+const symb: Symbol = Symbol('foo');
+
+// use a proper function type
+const func: Function = () => 1;
+
+// use safer object types
+const lowerObj: object = {};
+
+const capitalObj1: Object = 1;
+const capitalObj2: Object = { a: 'string' };
+
+const curly1: {} = 1;
+const curly2: {} = { a: 'string' };
+```
+
+Examples of **correct** code with the default options:
+
+```ts
+// use lower-case primitives for consistency
+const str: string = 'foo';
+const bool: boolean = true;
+const num: number = 1;
+const symb: symbol = Symbol('foo');
+
+// use a proper function type
+const func: () => number = () => 1;
+
+// use safer object types
+const lowerObj: Record<string, unknown> = {};
+
+const capitalObj1: number = 1;
+const capitalObj2: { a: string } = { a: 'string' };
+
+const curly1: number = 1;
+const curly2: Record<'a', string> = { a: 'string' };
+```
+
 ## Compatibility
 
 - TSLint: [ban-types](https://palantir.github.io/tslint/rules/ban-types/)