Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: recommended config #69

Merged
merged 4 commits into from
Jul 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 41 additions & 19 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,57 +6,79 @@
[![Npm](https://img.shields.io/npm/v/eslint-plugin-deprecation.svg)](https://www.npmjs.com/package/eslint-plugin-deprecation)
[![Npm Downloads](https://img.shields.io/npm/dt/eslint-plugin-deprecation.svg)](https://www.npmjs.com/package/eslint-plugin-deprecation)
![Size](https://badgen.net/bundlephobia/minzip/eslint-plugin-deprecation)
[![Licence](https://img.shields.io/npm/l/eslint-plugin-deprecation.svg?maxAge=2592000)](https://github.com/gund/eslint-plugin-deprecation/blob/master/LICENSE)
[![License](https://img.shields.io/npm/l/eslint-plugin-deprecation.svg?maxAge=2592000)](https://github.com/gund/eslint-plugin-deprecation/blob/master/LICENSE)
[![semantic-release](https://img.shields.io/badge/%20%20%F0%9F%93%A6%F0%9F%9A%80-semantic--release-e10079.svg)](https://github.com/semantic-release/semantic-release)

> ESLint rule that reports usage of deprecated code
> An [ESLint](https://eslint.org/) rule that reports usage of deprecated code.

## Prerequisites

This plugin only works with `@typescript-eslint/parser`.
If you already use [TypeScript](https://www.typescriptlang.org/) and one or more rules from the [`typescript-eslint`](https://typescript-eslint.io/) plugin, then `eslint-plugin-deprecation` will work out of the box without any additional dependencies or special configuration specified in this section. (This is because `@typescript-eslint/plugin` automatically contains `@typescript-eslint/parser` and your ESLint should already be configured with the `parserOptions` to work properly with TypeScript.)

Which means that you should install dev deps:
Otherwise, in order for you to use this plugin, you must also install the following dependencies:

- `@typescript-eslint/parser`
- `typescript`
- `@typescript-eslint/parser`

For example, if you use the `npm` package manager, then you would run the following command in the root of your project:

```sh
npm install --save-dev typescript @typescript-eslint/parser
```

Then configure ESLint to parse TypeScript and include type information:
Next, you must configure ESLint to parse TypeScript and include type information:

```jsonc
{
"parser": "@typescript-eslint/parser",
"parserOptions": {
"ecmaVersion": 2020,
"sourceType": "module",
"project": "./tsconfig.json" // <-- Point to your project's tsconfig.json or create new one
"project": "./tsconfig.json" // <-- Point to your project's "tsconfig.json" or create a new one.
}
}
```

## Install

Install the plugin
For example, if you use the `npm` package manager, then you would run the following command in the root of your project:

```
npm i -D eslint-plugin-deprecation
```sh
npm install --save-dev eslint-plugin-deprecation
```

## Setup

Now add deprecation plugin and rule to your `.eslintrc`:
### Using the `recommended` Config

The easiest way to use this plugin is to extend from the `recommended` config, like this:

```jsonc
{
"plugins": ["deprecation", ...],
"rules": {
"deprecation/deprecation": "warn", // or "error" to have stricter rule
...
}
"extends": [
"plugin:deprecation/recommended",
],
}
```

Now eslint will report all deprecated code that you use!
The `recommended` config will enable the plugin and enable the rule with a value of `error`.

### Manually Enable the Plugin and Rule

If you don't want to use the `recommended` config for some reason, you can accomplish the same thing by specifying the following config:

```jsonc
{
"plugins": [
"deprecation",
],

"rules": {
"deprecation/deprecation": "error",
},
}
```

---
## Credits

_NOTE:_ This rule was ported from https://github.com/SonarSource/SonarJS repository.
This rule was originally ported from [SonarJS repository](https://github.com/SonarSource/SonarJS).
3 changes: 3 additions & 0 deletions src/configs/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import recommended from './recommended';

export { recommended };
8 changes: 8 additions & 0 deletions src/configs/recommended.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
const recommended = {
plugins: ['deprecation'],
rules: {
'deprecation/deprecation': 'error',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use warning as a recommended configuration and let users to opt in into a more strict mode manually.

Copy link
Contributor Author

@Zamiell Zamiell Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the industry best-practice is to use error.

For example, see the ESLint recommended rules, which contain no warnings.

Also, see the TypeScript-ESLint recommended rules, which contain no warnings.

Also, see the Unicorn recommended rules, which contain no warnings.

And so on.

Personally, in my projects, I use eslint-plugin-only-warn, which converts all errors to warnings, making the distinction superfluous. The reason is because I like red squigglys to be from the TypeScript compiler, and yellow squigglys to be from ESLint, which provides a nice delineation.

Copy link
Owner

@gund gund Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I agree that majority of eslint rules should be errors but deprecation for me seems more like a thing to keep an eye on and not necessarily a bad thing which is fine to have in your code and not fail your CI.
And simply forcing ppl to ignore their deprecations because the rule is too strict is eventually going to lead to a more deprecated code being accumulated and lost track of as eslint will no longer report those.
I know it's probably opinionated but I guess that's the whole point of "recommended" config anyway =)

Copy link
Contributor Author

@Zamiell Zamiell Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I agree that majority of eslint rules should be errors

I guess the point of my previous explanation was that I think that all ESLint rules should be errors, not just "most". If users want to explicitly not follow ESLint best practice and use warn, then they should do that by not using the recommended config and manually configure the rule themselves.

not necessarily a bad thing which is fine to have in your code and not fail your CI.

I respectfully disagree! =p I think failing in CI is a fantastic time to know about deprecations. What is the alternative? For the ESLint warning to only show up in VSCode if one of the programmers on your project happens to open the file?

  • What if its a file that hasn't been touched in years? What are the chances that someone is going to randomly open it in their code editor?
  • Or, alternatively, what are the chances that someone is going to randomly notice the warning in CI by reading CI logs for a non-failed CI run?
  • It seems much more likely that they will first discover the warning when reading the CI logs for an actual failed CI run, which causes unnecessary noise and actively wastes someone's time when trying to get to the root cause of the issue causing the actual failure.
  • Or, alternatively, what are the chances that someone is going to manually run lint on their local computer to discover the warning when linting is already automatically set up in CI?

Copy link
Contributor Author

@Zamiell Zamiell Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More thoughts:

I think a common workflow for TypeScript projects is to update dependencies in package.json, make a commit, push to GitHub, and then see if CI passes. (TypeScript compiler might fail on changed/deleted library functions, and if TypeScript compiler passes all the code is probably okay.)

This is the exact moment in time where I want to first find out that my library usage is deprecated. By not having CI fail with a deprecation, the deprecation is going to be hidden until N days in the future, when I am busy dealing with other things, and reverting deps in the project might not be possible anymore.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I see your points, and I do agree that errors are better for discovery, because literally nobody ever reads warnings if everything passes.
But deprecations are a bit different can of worms than a simple code style rule which you may not be able to resolve that easily. And because deprecations are still a valid code that will work until some time in the future, it is totally valid to keep it for now until you have plans to prepare for a future version of whatever you are using.
This is why I see it as a warning mostly - it's okay to use that piece of code but you have to know that at some point in the future you will have to refactor. This is the definition of the warning to me. And I would not want to add those "eslint-ignore" comments just because the rule is going to crash CI.
If you are still convinced that errors are better for deprecations - I'm fine to merge it as is, but personally I would not use "recommended" version in my projects 😁

Copy link
Contributor Author

@Zamiell Zamiell Jul 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree that errors are better for discovery

In my mind, the primary purpose of eslint-plugin-deprecation is discovery. I want to discover when library functions become deprecated, so that I can change my code. All the other points don't matter as much as discovery.

I would prefer to merge as is, but we can leave this issue open for a while to see what other developers and users of eslint-plugin-deprecation think. I predict that most other people in the ecosystem will not use warn for this rule (or for any other rule for that matter), but I'm open to other perspectives.

Copy link

@SchroederSteffen SchroederSteffen Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the rule with warn, but execute ESLint with --max-warnings 0.
Note that using max-warnings 0 isn't only about deprecations. We also think that warnings can easily be overlooked, especially when trusting the CI. Thus, we fail on warnings.
I still think that deprecations should be warnings, because there may not be an immediate action required and solving a deprecation could be postponed by increasing the --max-warnings without ignoring the finding altogether.

},
};

export default recommended;
3 changes: 2 additions & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as rules from './rules';
import * as configs from './configs';

export { rules };
export { configs, rules };