mightyiam / eslint-config-love

A TypeScript ESLint config that loves you
MIT License
776 stars 69 forks source link

Add compatible TSConfig base #1020

Open razor-x opened 1 year ago

razor-x commented 1 year ago

What version of this package are you using?

{
  "devDependencies": {
    "eslint": "^8.9.0",
    "eslint-config-standard-with-typescript": "^26.0.0",
    "typescript": "^4.9.4"
  }
}

What problem do you want to solve?

When using TypeScipt with Standard, some linting rules configured here can conflict with checks defined in tsconfig.json.

  1. If a user enables conflicting rules, then the linting step might pass, but the compile step might fail.
  2. If a user enables matching rules, the linter and compiler will produce duplicated errors creating noise and confusion: is this one problem or two?
  3. Ideally, the user should disable the TypeScipt checks already handled by Standard.
  4. There is no simple way for the user to know what TypeScipt options to set for compatibility with Standard.
  5. Even if the user finds them manually, they may change unexpectedly when this package updates rules or TypeScipt defaults change.

What do you think is the correct solution to this problem?

  1. Add a tsconfig base file to this project or directly PR to https://github.com/tsconfig/bases/tree/main/bases
  2. Update the usage instructions here to suggest the user extend their typescript config with this base file.

This base file disable the TS checks that are already handled by Standard.

{
  "compilerOptions": {
    "allowUnreachableCode": true,
    "allowUnusedLabels": true,
    "noFallthroughCasesInSwitch": false,
    "noUnusedLocals": false
  },
  "$schema": "https://json.schemastore.org/tsconfig",
  "display": "Standard"
}

Are you willing to submit a pull request to implement this change?

Yes.

mightyiam commented 1 year ago

Thank you for caring enough to report this. I'm not sure that this is worth the user's effort during setup. How do you feel?

razor-x commented 1 year ago

@mightyiam I think the smallest useful implementation of this feature is an added section to the README that documents the tsconfig options that are duplicated or conflicting here.

I agree this is not needed to get setup, but without this information provided by this project, the user will need to go though each tsconfig option one at a time and test which options must be set in order to avoid the described problems.


Related TSConfig options

The following TSConfig compiler options are handled by this ESLint config and may be safely disabled to avoid double reporting by TypeScript:


{
  "compilerOptions": {
    "allowUnreachableCode": true,
    "allowUnusedLabels": true,
    "noFallthroughCasesInSwitch": false,
    "noUnusedLocals": false
  }
}
mightyiam commented 1 year ago
  1. If a user enables conflicting rules, then the linting step might pass, but the compile step might fail.

Regarding point number 1, we can't think of such an example. Can you?

  1. If a user enables matching rules, the linter and compiler will produce duplicated errors creating noise and confusion: is this one problem or two?

The example compilerOptions you provided seem to all be of the kind that could cause point number 2 — duplication of error reporting.

Please confirm.

razor-x commented 1 year ago

@mightyiam I think you are right, the current eslint-config-standard-with-typescript ruleset is compatible with the strictest typescript checks enabled. So (1) is just a theoretical problem with eslint rules conflicting with the TS checks.

The example compilerOptions you provided seem to all be of the kind that could cause point number 2 — duplication of error reporting.

Yea, this should be the case, and my initial motivation for finding the overlapping cases.

mightyiam commented 1 year ago
  1. If we provide such a tsconfig to extend from, how many users would actually care enough to use it? The value proposition seems low enough that I would feel somewhat uncomfortable having the users spend time reading the documentation of this provided tsconfig feature.
  2. TypeScript allows extending from only a single tsconfig file. So that would prevent at least a significant portion of users from even using this feature.
  3. While implementation of this seems quite trivial, testing this feature might prove tricky. That, I expect to be the majority of the development cost.

@razor-x do you feel it's worth it considering these?

razor-x commented 1 year ago

If we provide such a tsconfig to extend from, how many users would actually care enough to use it?

I have no way of knowing or evaluating this. I can only speak from my experience.

The value proposition seems low enough that I would feel somewhat uncomfortable having the users spend time reading the documentation of this provided tsconfig feature.

I agree it may not belong inline with the setup instructions, however having this information available, assuming it stays up to date, can only help those who look for it.

TypeScript allows extending from only a single tsconfig file. So that would prevent at least a significant portion of users from even using this feature.

Yes, that is an unfortunate limitation. More likely then, a user would integrate the existing options into their own config and be responsible for updating them when they update these rules. It seems likely that any change to these rules that affected the existing compatibility with TSConfig would be breaking, so that is not an unreasonable ask from the user in my opinion.

While implementation of this seems quite trivial, testing this feature might prove tricky. That, I expect to be the majority of the development cost.

This is the most compelling point: having outdated documentation is often worse than no documentation. Having a way to test the compatibility in CI is a good prerequisite for adding this. I don't have a solution at the moment, but open to thinking about it, or implementing any suggestions.

mightyiam commented 1 year ago

@razor-x, would you enjoy joining us in one of these sessions to think about this, or possibly help us decide on the next rules?

razor-x commented 1 year ago

Sorry for disappearing on this. Thanks for the invite, I can try to join a session, but the timing is difficult for me.

About this comment:

TypeScript allows extending from only a single tsconfig file. So that would prevent at least a significant portion of users from even using this feature.

TypeScript 5.0.is out and has an update that resolves this constraint! https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#supporting-multiple-configuration-files-in-extends

I'm wondering if this project could adopt opinions on some of the other TypeScript "style" options to further follow the path of Standard's "No configuration / no bikeshedding". So this package is "eslint rules + compatible TypeScript rules"

So this module would provide a base config like this:


{
  "compilerOptions": {
    // enables a lot of checks, but maybe out of scope for this?
    "strict": true,

    // Effects output only, maybe out of scope?
    "newLine": "lf",

    // New option, recommend but maybe out of scope?
    "verbatimModuleSyntax": true,

    // Recommend checks to enable by TypeScript docs
    "exactOptionalPropertyTypes": true,
    "noImplicitOverride": true,
    "noImplicitReturns": true,
    "noPropertyAccessFromIndexSignature": true,
    "noUncheckedIndexedAccess": true,
    "noUnusedParameters": true,
    "forceConsistentCasingInFileNames": true,

    // Inverse of these are recommend by TypeScript docs, but already handled by eslint-config-standard-with-typescript
    "allowUnreachableCode": true,
    "allowUnusedLabels": true,
    "noFallthroughCasesInSwitch": false,
    "noUnusedLocals": false
}
mightyiam commented 1 year ago

Thank you. We'll take a look at this. Highest priority is currently #1149.