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

[disable-type-checked-config] missing @typescript-eslint/explicit-function-return-type rule #8955

Open
2 tasks done
OlivierZal opened this issue Apr 19, 2024 · 11 comments
Open
2 tasks done
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin preset config change Proposal for an addition, removal, or general change to a preset config triage Waiting for maintainers to take a look

Comments

@OlivierZal
Copy link

OlivierZal commented Apr 19, 2024

Before You File a Proposal Please Confirm You Have Done The Following...

Description

@typescript-eslint/explicit-function-return-type is not a "type-checked" rule but could belong to the disableTypeChecked config since it does not make sense to require a return type in a context where types are not checked / mandatory, e.g. in .js files.

Impacted Configurations

disableTypeChecked config

Additional Info

No response

@OlivierZal OlivierZal added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin preset config change Proposal for an addition, removal, or general change to a preset config triage Waiting for maintainers to take a look labels Apr 19, 2024
@OlivierZal
Copy link
Author

Closing this issue after @bradzacher closed the related PR.

@bradzacher
Copy link
Member

bradzacher commented Apr 19, 2024

@OlivierZal we can still discuss this idea here! We just like to start with an issue so we don't put the cart before the horse and have you burn time on work that might not be accepted.

@OlivierZal OlivierZal closed this as not planned Won't fix, can't repro, duplicate, stale Apr 19, 2024
@OlivierZal OlivierZal reopened this Apr 20, 2024
@OlivierZal
Copy link
Author

Hi @bradzacher, I refined the description.

@kirkwaiblinger
Copy link
Member

TLDR - I don't think you want to use disable-type-checked for disabling typescript-eslint rules in *.js files. I would just disable all typescript-eslint rules.


I guess it's a question of what the purpose of the disable-type-checked config is. The example given in the docs shows it as being used to disable type-checked rules for **/*.js files.

To me, that seems wrong. Granted our "New Rule" issue template says "My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.", if a rule

  1. deals with types, it might partially work in a .js file (e.g. no-floating-promises works).
  2. deals with TS syntax, it definitely should not be expected to work in a .js file. (see also parameter-properties false positive)

So, there should be no scenario where you'd only want typed lint rules disabled from JS files, right?

So, I come back to - that seems like a wrong use of the disable-type-checked config. I'd think instead, you'd simply... not want to enable typescript-eslint rules in the first place in a JS file. And disable-type-checked would be used for TS files you might have in your repo that you can't/don't want to set up with typed linting, or for removing type-aware rules out of a config that you've imported from somewhere else.

@bradzacher does that align with your understanding?

@bradzacher
Copy link
Member

bradzacher commented May 7, 2024

I would just disable all typescript-eslint rules.

I don't think that's a good summary, no.
Many of our rules work just fine in JS files!
Type-aware rules work just fine in JS files - so long as you're declaring half-decent JSDoc types they'll all work as expected. It's just that JS files are often lazily typed (if at all) and the rules do essentially function on "garbage in, garbage out".

IIRC there's only three rules which will explicitly not work in a JS file:

  • explicit-function-return-type
  • explicit-module-boundary-types
  • parameter-properties

Everything else will work just fine (and the TS-specific stuff just never matches).

There could be some value in having a config to disable the above 3 rules as in "disable-requiring-type-syntax" or something? That way you could declare like

export default tseslint.config({
  files: ["**/*.js"],
  extends: [tseslint.configs.disableRequiringTypeSyntax, tseslint.configs.disableTypeChecked],
});

@kirkwaiblinger
Copy link
Member

I don't think that's a good summary, no.

Lol, savage 😅 But, after reading your response, I think you're right 😆

Many of our rules work just fine in JS files! Type-aware rules work just fine in JS files - so long as you're declaring half-decent JSDoc types they'll all work as expected. It's just that JS files are often lazily typed (if at all) and the rules do essentially function on "garbage in, garbage out".

This is actually very cool! I haven't used the jsdoc-typed-js workflow yet, so I learned something new today.

I think this does prove the point that using disable-type-checked is not useful where what you would want is disable-rules-that-wont-work-in-js-files.

IIRC there's only three rules which will explicitly not work in a JS file:

  • explicit-function-return-type
  • explicit-module-boundary-types
  • parameter-properties

Looking around a bit, typedef should probably go in there too, since that's all about adding type annotations. Didn't find any other obvious examples.

There could be some value in having a config to disable the above 3 rules as in "disable-requiring-type-syntax" or something? That way you could declare like

export default tseslint.config({
  files: ["**/*.js"],
  extends: [tseslint.configs.disableRequiringTypeSyntax, tseslint.configs.disableTypeChecked],
});

+1


Can we take an action item to change the docs to not give **/*.js as the example for when to use disable-type-checked? Granted this discussion, that feels like a total red herring.

And we can split out the idea of disableRequiringTypeSyntax/disableRulesThatWontWorkInJSFiles to a separate issue?

@bradzacher
Copy link
Member

bradzacher commented May 7, 2024

Can we take an action item to change the docs to not give **/*.js as the example for when to use disable-type-checked?

The reason we use this as our example is because in modern TS codebases that's what most users want to do!

Having .js files is rare in a modern TS codebase and they're usually only there because you're using a tool that doesn't support TS (eg nodejs entrypoints or tooling config files).

The important bit is that because of their nature these files are rarely distributed to users and so people don't bother trying to create a tsconfig setup that matches those files.
Which is the difficult thing about type-aware linting - if you don't cover those files with a tsconfig then you get eslint failures on those files.

So the common user story as a modern TS codebase is "I have JS files for config but I don't want to try and make them work with type-aware linting so I want to disable type-aware linting altogether".

But ofc there are some codebases that are all-in on JS with JSDoc plus TS and type-aware linting. So in those codebases the above story does not apply. These codebases are the very rare case though cos jsdoc typing is cumbersome AF.

Its worth noting that with the new project service we've added better support for this usecase as these files can be automatically covered by a "default program". But it's still likely that people don't want type-aware rules because JSDoc typing is cumbersome AF and often times you haven't bothered grabbing @types packages for the dev deps - or there aren't even types for the packages (eg see the current state of eslint plugins and flat config files)

@OlivierZal
Copy link
Author

OlivierZal commented May 7, 2024

@bradzacher,

Having .js files is rare in a modern TS codebase and they're usually only there because you're using a tool that doesn't support TS (eg nodejs entrypoints or tooling config files).

I totally agree: I have only TS files... except the eslint flat config, which as far as I know must be a file named exactly eslint.config.js (kind of irony, isn't it?), and that's in this file I want to disable useless type-related eslint errors, although I want them elsewhere.

@kirkwaiblinger
Copy link
Member

And, fair enough, but wouldn't it more helpful to the user to achieve that goal having a doc section that looks (very very) roughly like


Working with a mixed JS/TS codebase

Many of our rules actually work just fine in JS files!

However, some of our rules will cause spurious errors if they're applied to JS files, so you'll need to disable them.

export default tseslint.config(
  eslint.configs.recommended,
  ...tseslint.configs.recommendedTypeChecked,
  {
    languageOptions: {
      parserOptions: {
        project: true,
        tsconfigDirName: import.meta.dirname,
      },
    },
  },
+  {
+    // These rules will try to enforce TS syntax which is illegal inside a JS file
+    // maybe one day this will be included in a utility config rather than listed out individually
+    files: ['**/*.js'],
+    rules: [
+         "@typescript-eslint/explicit-function-return-types": "off",
+         "@typescript-eslint/parameter-properties": "off",
+         "@typescript-eslint/explicit-module-boundary-types": "off",
+         "@typescript-eslint/typedef": "off",
+    ]
+  },
);

Depending on your setup, our type-aware rules may cause problems as well. We provide the utility disable-type-checked config to help you disable them too

export default tseslint.config(
  eslint.configs.recommended,
  ...tseslint.configs.recommendedTypeChecked,
  {
    languageOptions: {
      parserOptions: {
        project: true,
        tsconfigDirName: import.meta.dirname,
      },
    },
  },
  {
    // These rules will try to enforce TS syntax which is illegal inside a JS file
    files: ['**/*.js'],
    rules: [
         "@typescript-eslint/explicit-function-return-types": "off",
         "@typescript-eslint/parameter-properties": "off",
         "@typescript-eslint/explicit-module-boundary-types": "off",
         "@typescript-eslint/typedef": "off",
    ]
  },
+  {
+    files: ['**/*.js'],
+    ...tseslint.configs.disableTypeChecked,
+  }
);

Alternately, if you use JS doc types in your JS files (helpful-link-to-more-info-on-this-for-people-who-don't-know-what-that-is), our type-aware rules will work, as long as the corresponding tsconfig is included in the eslint config.


The issue is that the way the docs are currently written, is that it makes it seem like disable-type-checked is all that you need to do to get the setup working with js files, which is how we got to this issue report in the first place.

@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented May 8, 2024

also, lol on the eslint.config.js being the culprit 😆

@bradzacher
Copy link
Member

The issue is that the way the docs are currently written, is that it makes it seem like disable-type-checked is all that you need to do to get the setup working with js files

If you're using the recommended config - then disable-type-checked is all you need 🙂

It's only when people branch out into custom configs that they need more!
For those custom usecases we do have it documented to some degree (docs that likely need updating 😅):

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 preset config change Proposal for an addition, removal, or general change to a preset config triage Waiting for maintainers to take a look
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants