Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Configs: Have recommended/strict configs include lesser configs, and simplify type checked names #6019

Discussion options

Description

Edited December 8th, 2022: updated to include @aaronadamsCA's #6019 (comment) and @robertknight's #6014 (comment) to this proposal.

Context

Right now, no config in the following list of configs provided with typescript-eslint includes any other config in that list:

  • plugin:@typescript-eslint/recommended
  • plugin:@typescript-eslint/recommended-requiring-type-checking
  • plugin:@typescript-eslint/strict

That means if you want, say, the strictest, you must enable all three (https://typescript-eslint.io/docs/linting/configs):

{
  "extends": [
    "plugin:@typescript-eslint/recommended",
    "plugin:@typescript-eslint/recommended-requiring-type-checking",
    "plugin:@typescript-eslint/strict"
  ]
}

That's very inconvenient -and sometimes even confusing- for end users.

Breaking Change: Including Lesser Configs

Proposal: how about we have each config in that list also include any previous config in the list? That way if you want, say, the strict ruleset, you would only need to enable it, not recommended-* ones:

{
  "extends": [
    "plugin:@typescript-eslint/strict"
  ]
}

Associated Enhancement: More Delineated Configs

We've received occasional user feedback (e.g. #6014 (comment)) that our recommended rulesets also include some stylistic rules. The ESLint community has generally moved away from the old 2010s-era eslint-config-airbnb practice of doing that. I agree that we should separate those out.

This proposal suggests we split our recommended configurations into:

  • Functional rule configurations, for best best practices and code correctness:
    • recommended: Recommended rules that you can drop in without additional configuration.
    • recommended-type-checked: Additional recommended rules that require type information.
    • strict: Additional strict rules that can also catch bugs but are more opinionated than recommended rules.
    • strict-type-checked: Additional strict rules require type information.
  • Stylistic rule configurations:
    • stylistic: Stylistic rules you can drop in without additional configuration.
    • stylistic-type-checked: Additional stylistic rules that require type information.

You can see the equivalent code changes in #5251.

Description of Breaking Changes

In other words, there are three changes being proposed here:

  • Reworking what's existing in the existing recommended, recommended-requiring-type-checking, and strict configs: this is a breaking change
  • Adding new strict-type-checked, sylistic, and stylistic-type-checked configurations: this is not a breaking change
  • Renaming recommended-requiring-type-checking to recommended-type-checked: this would be a breaking change, except recommended-requiring-type-checking, will be left as an alias for its new config name, recommended-type-checked.
    • We'll eventually breaking change away the old alias - perhaps in v7.

Thanks @shawnmcknight for a great Twitter discussion on phrasing the breaking changes!

You must be logged in to vote

Replies: 2 comments · 6 replies

Comment options

Could I suggest a change?

Is it possible we could get a strict-requiring-type-checking that includes recommended-requiring-type-checking, and then strict would only include recommended?

I'd love to extend the strict recommendation, but right now for performance purposes we try not to connect rules that require type information to the IDE. If I could just extend strict for the IDE and then add strict-requiring-type-checking for the check scripts, it would cut down on our maintenance a ton.

I know it's a separate request, but I wanted to mention it here first since it's in direct opposition to half of this change.

You must be logged in to vote
2 replies
@JoshuaKGoldberg
Comment options

I wish we could thread existing discussion comments, since migrating from issues puts them all root-level 🙃 but responding in #6019 (reply in thread): yes!

@JoshuaKGoldberg
Comment options

JoshuaKGoldberg Dec 8, 2022
Maintainer Author

@aaronadamsCA I merged this into the OP and so will hide this thread. [Edit: unhid so folks can see this context] Thanks! 😄

Comment options

I definitely agree that splitting out strict-requring-type-checking might be a good idea.
One of the reasons we separated recommended and recommended-requiring-type-checking is so that people could chose to lint with type information in specific situations for perf reasons.

So i think it might make sense to continue this theme for strict as well?

You must be logged in to vote
4 replies
@JoshuaKGoldberg
Comment options

Agreed, I really like this. I've also been noodling on getting a less verbose name. The closest I've come up with has been *-type-checked. So that makes four configs:

  • recommended
  • recommended-type-checked
  • strict
  • strict-type-checked

I'll update the PR.

@bradzacher
Comment options

do we want to rename the configs? that seems like a pretty major breaking change that would impact most of our users.
Personally I'm averse to such sweeping breakages unless there's significant value to be gained - I don't think that saving one word is a big enough win to justify.

@JoshuaKGoldberg
Comment options

I like renaming them to draw attention to big shifts, like going from three configs to six. It forces users to take stock of their configs. Especially if strict ends up being weaker in v6 than it is now, for having type-checked rules moved to strict-type-checked.

To mitigate pain, I'd propose we:

  • Leave recommended-requiring-type-checking alive as an alias for recommended-type-checked
  • Go through and send PRs to major packages (we can use Sourcegraph to query for them!)
  • Make it log a console warning in v7
  • Remove it in v8
@JoshuaKGoldberg
Comment options

JoshuaKGoldberg Dec 8, 2022
Maintainer Author

Chatted privately - we're 👍 to go for now!

I merged this into the OP and so will hide this thread. Edit: unhid so folks can see this context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin breaking change This change will require a new major version to be released accepting prs Go ahead, send a pull request that resolves this issue preset config change Proposal for an addition, removal, or general change to a preset config
3 participants
Converted from issue

This discussion was converted from issue #5204 on November 17, 2022 15:54.

Morty Proxy This is a proxified and sanitized view of the page, visit original site.