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

Add compatible TSConfig base #1020

Open
razor-x opened this issue Jan 6, 2023 · 9 comments
Open

Add compatible TSConfig base #1020

razor-x opened this issue Jan 6, 2023 · 9 comments

Comments

@razor-x
Copy link

razor-x commented Jan 6, 2023

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
Copy link
Owner

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
Copy link
Author

razor-x commented Jan 10, 2023

@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
Copy link
Owner

  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
Copy link
Author

razor-x commented Jan 18, 2023

@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
Copy link
Owner

  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
Copy link
Author

razor-x commented Jan 27, 2023

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
Copy link
Owner

@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
Copy link
Author

razor-x commented Mar 29, 2023

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

2 participants