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

Bug: rules from @typescript-eslint/eslint-recommended cannot be applied to files with custom file extensions #8607

Closed
4 tasks done
chancancode opened this issue Mar 6, 2024 · 12 comments
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin package: typescript-eslint Issues related to the typescript-eslint package wontfix This will not be worked on

Comments

@chancancode
Copy link

chancancode commented Mar 6, 2024

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Issue Description

Tl;DR

This reproduces the problem that in .gts files, conflicting or redundant eslint:recommended rules, such as getter-return, are not disabled.

// Minimal .eslintrc.js
module.exports = {
  root: true,
  parser: "ember-eslint-parser",
  plugins: ["ember", "@typescript-eslint"],
  extends: [
    // Enables the `getter-return` rule
    "eslint:recommended",

    // Disables the `getter-return` rule
    "plugin:@typescript-eslint/eslint-recommended",
  ],
};
// foo.ts – eslint passes
export default class Foo {
  get foo(): true | undefined {
    if (Math.random() > 0.5) {
      return true;
    }
  }
}
// foo.gts – exactly the same file, only different extension, eslint fails
export default class Foo {
  // error: Expected getter 'foo' to always return a value (getter-return)
  get foo(): true | undefined {
    if (Math.random() > 0.5) {
      return true;
    }
  }
}

I believe this affects any files that does not have a standard TypeScript extension, so I think it may be a problem for .vue, etc, as well.

Background

Ember uses .gjs and .gts files for single-file components. It has a small piece of syntax extension (<template>...</template>) to JS/TS that requires a different parser (ember-eslint-parser), which is configured here. It does a small amount of pre-processing to remove the custom syntax extension and then subsequently delegates to @typescript/eslint-parser.

However, .gjs and .gts are a strict superset of JS/TS syntax, and as long as you don't use the custom syntax extension, they behave identically. In other words, any valid .js/.ts files are a valid .gjs/.gts file.

I don't believe the issue here has to do with the parser/parsing, so these should ultimately be irrelevant for this discussion. The examples in this reproduction are simultaneously legal TS/GTS code (should be parsable with the stock parser if you force it).

The Issue

The issue is that the @typescript-eslint/eslint-recommended config has a hardcoded list of file extensions:

/**
 * This is a compatibility ruleset that:
 * - disables rules from eslint:recommended which are already handled by TypeScript.
 * - enables rules that make sense due to TS's typechecking / transpilation.
 */
import type { ClassicConfig } from "@typescript-eslint/utils/ts-eslint";

export = {
  overrides: [
    {
      files: ["*.ts", "*.tsx", "*.mts", "*.cts"],
      rules: {
        "constructor-super": "off", // ts(2335) & ts(2377)
        "getter-return": "off", // ts(2378)
        "no-const-assign": "off", // ts(2588)
        "no-dupe-args": "off", // ts(2300)
        "no-dupe-class-members": "off", // ts(2393) & ts(2300)
        "no-dupe-keys": "off", // ts(1117)
        "no-func-assign": "off", // ts(2630)
        "no-import-assign": "off", // ts(2632) & ts(2540)
        "no-new-symbol": "off", // ts(7009)
        "no-obj-calls": "off", // ts(2349)
        "no-redeclare": "off", // ts(2451)
        "no-setter-return": "off", // ts(2408)
        "no-this-before-super": "off", // ts(2376) & ts(17009)
        "no-undef": "off", // ts(2304) & ts(2552)
        "no-unreachable": "off", // ts(7027)
        "no-unsafe-negation": "off", // ts(2365) & ts(2322) & ts(2358)
        "no-var": "error", // ts transpiles let/const to var, so no need for vars any more
        "prefer-const": "error", // ts provides better types with const
        "prefer-rest-params": "error", // ts provides better types with rest args over arguments
        "prefer-spread": "error", // ts transpiles spread to apply, so no need for manual apply
      },
    },
  ],
} satisfies ClassicConfig.Config;

Linking/embedding the version from v6.21.0 here, because the work to support flat config obfuscated the code/problem but I think they fundamentally suffer the same issue.

I gathered that the intention here is to make the kind of simple/minimal legacy config (like the snippet I had at the top) Just Work™ – it will turn off the conflicting rules for TypeScript files but leave it alone for JavaScript files.

But suppose you do this instead:

module.exports = {
  root: true,
  overrides: [
    {
      files: ["*.ts", "*.gts"],
      parser: "ember-eslint-parser",
      plugins: ["ember", "@typescript-eslint"],
      extends: [
        // Enables the `getter-return` rule
        "eslint:recommended",

        // Disables the `getter-return` rule
        "plugin:@typescript-eslint/eslint-recommended",
      ],
    },
  ],
};

The intention here seems pretty straightforward – to apply the same set of rules to both .ts and .gts files. It looks like the should work, but it still doesn't. Because the config being extended has files included in it the portion of the rules ended up only applying to .ts files but not for .gts files.

More bizarrely, if you extend from plugin:@typescript-eslint/recommended or similar (either instead or in addition to @ts-e/eslint-recommended), those additional TypeScript-specific rules are applied to both types of files, because it does not include a file list, but the portion that happens to be defined in @ts-e/eslint-recommended are still excluded for .gts files.

Flat Config?

I think flat configs fundamentally have the same problem, but I suppose because it is "Just JavaScript" you can "fix" it.

But even that will require deeply prying into the default config objects.

Conclusions

Not sure how to coordinate/workaround this problem, other than duplicating the set of rules in @ts-e/eslint-recommended somewhere (either in the application .eslintrc.js, or as plugin:ember/gte-slint-recommendeds etc).

Is there some alternative factoring of the configs so that these rules won't be restricted just those hardcoded list of file extensions?

Reproduction Repository Link

https://github.com/chancancode/eslint-gts-reproduction

Repro Steps

  1. clone the repo
  2. pnpm install
  3. pnpm lint in packages/*

Versions

package version
@typescript-eslint/eslint-plugin 6.20.0 (or higher)
TypeScript 5.3.3 (or higher)
ESLint 8.57.0 (or higher)
node 20.10.0 (or higher)
@chancancode chancancode added bug Something isn't working triage Waiting for team members to take a look labels Mar 6, 2024
@bradzacher
Copy link
Member

The reason that it is defined like this is two fold:

(1) because these rules should only be turned off on TS files - not JS files.
We know these rules are safe to turn off because TS checks them in TS files. But JS files are, more often than not, not covered by TS. So turning of those rules in JS files would lead to bad code being allowed.

(2) the way the legacy config system is designed it will only lint JS extensions by default. By using overrides we tell eslint to expand the allowed extensions to those listed. This means users don't need to pass --ext ts,tsx from the command line - removing a major pain point.


With flat configs (2) no longer applies because eslint no longer restricts extensions. But (1) still applies just as strongly.

Additionally with flat configs as you mention it's just JS so you can easily apply them to your custom extensions:

import tseslint from 'typescript-eslint';

export default [
  {
    ...tseslint.configs.eslintRecommended,
    files: ["**/*.gts"],
  }, 
];

@bradzacher
Copy link
Member

bradzacher commented Mar 6, 2024

Ultimately I don't think there is a strong enough motivation for us to change the state here.

I see that ember-eslint-parser is very new (first release Dec last year) and is very small relative to us (22k vs 32m weekly DLs - about 0.06% of our userbase assuming every single user also writes gts files).

It doesn't seem like us trying to fandangle a solution for legacy configs to solve for a brand new project is a good thing to do. I'm satisfied with the advice "use flat configs and spread the config". It's probably worth the ember plugin setting up their recommended flat config to do this automatically for gts files.

Cc @typescript-eslint/triage-team

@chancancode
Copy link
Author

Additionally with flat configs as you mention it's just JS so you can easily apply them to your custom extensions

I am not sure that works (I haven't tried it), but the reason I said "I think flat configs fundamentally have the same problem" is that I believe tseslint.configs.eslintRecommended expands into something like this:

[
  {
    languageOptions: { parser: ..., parserOptions: ... },
    plugins: { '@typescript-eslint': ... },
  },
  {
    files: [ '**/*.ts', '**/*.tsx', '**/*.mts', '**/*.cts' ],
    rules: {
      'constructor-super': 'off',
      'getter-return': 'off',
      'no-const-assign': 'off',
      'no-dupe-args': 'off',
      'no-dupe-class-members': 'off',
      'no-dupe-keys': 'off',
      'no-func-assign': 'off',
      'no-import-assign': 'off',
      'no-new-symbol': 'off',
      'no-obj-calls': 'off',
      'no-redeclare': 'off',
      'no-setter-return': 'off',
      'no-this-before-super': 'off',
      'no-undef': 'off',
      'no-unreachable': 'off',
      'no-unsafe-negation': 'off',
      'no-var': 'error',
      'prefer-const': 'error',
      'prefer-rest-params': 'error',
      'prefer-spread': 'error',
    },
  },
];

(this was last checked on 6.20 though)

Unless that has changed, I don't think that splat works. Since it is Just JavaScript™, there are ultimately ways to find it and change things, but I'm not sure if you think that's something we should rely on? (Either in the app config or in some shared library.)

I see that ember-eslint-parser is very new

Definitely! To be clear, not asking/expecting that you change the list of hardcoded extensions to include .gts (though if you are happy to do it eventually then I'm not complaining either!). Seeing that this problem would also affect .vue and other other custom extensions, I was hoping to see if there are some guidance or change/refactor that we can make to extract these config, so that they can be consumed in a way that works for non-.ts files as well.

@chancancode
Copy link
Author

Ah, I see I misread the snippet you typed. Yes, spatting onto eslintRecommended should work. However, in practice, you likely want to extend from one of the other configs, say, tseslint.configs.recommended.

So we would do:

export default [
  {
    ...tseslint.configs.eslintRecommended,
    files: ["**/*.gts"],
  },
  ...tseslint.configs.recommended
];

I think this will end up including the eslintRecommended snippet twice in the array, the first time with the "fixed" extensions, the second time with the original ones, but maybe that is ok/acceptable?

@bradzacher
Copy link
Member

Seeing that this problem would also affect .vue and other other custom extensions

Vue isn't impacted because Vue templates are not "ts-like code with new syntax". Instead Vue templates are "HTML-like code with pure TS inside <script type='ts'> tags.

The vue-eslint-parser runs a pre-processor which extracts the TS from inside the script tags and then eslint lints those fragments as .ts extension files.

Markdown is done in much the same way.

To my knowledge ember is the first language to lint itself in this way.

this was last checked on 6.20 though

We didn't have any flat configs until v7 when we first released the typescript-eslint package.

I think this will end up including the eslintRecommended snippet twice in the array, the first time with the "fixed" extensions, the second time with the original ones, but maybe that is ok/acceptable?

ESLint doesn't care. As far as it is concerned they're two completely independent and isolated configs because they apply to different file sets.

@chancancode
Copy link
Author

Cool, thanks for the help/replies! Closing for now as there doesn't seem to be anything actionable from this thread, I'll try it out and report back if I run into issues implementing this approach.

@runspired
Copy link

runspired commented Mar 6, 2024

To my knowledge ember is the first language to lint itself in this way.

Don't forget tsx and jsx are their own framework-specific languages ;)

@JoshuaKGoldberg
Copy link
Member

Don't forget tsx and jsx are their own framework-specific languages ;)

Nit: no, JSX is an XML-like syntax extension that happens to be used by many frameworks. Preact, React, Solid, etc. all happen to use JSX.

The distinction is important here because JSX is a general syntax extension recognized by TypeScript as a flavor of JavaScript. It's not framework-specific. General-purpose tools like typescript-eslint generally target the general syntaxes recognized by TypeScript, but not language-specific ones like Ember's.

Aside: TypeScript having four React-related values for its "jsx" compiler option is just a convenience for developers (that I personally disagree with the React-specific-ness of), not an indication that React is its own language.

@bradzacher bradzacher added wontfix This will not be worked on package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin package: typescript-eslint Issues related to the typescript-eslint package and removed triage Waiting for team members to take a look labels Mar 6, 2024
@runspired
Copy link

JSX is a general syntax extension recognized by TypeScript as a flavor of JavaScript

E.G. "someone sometime decided to hard code special cased support for a non-standard framework specific thing". JSX is not supported by most frameworks, nor would it have utility in being so because it is extremely coupled to react in multiple ways (ownership, maintenance, paradigms and general philosophy). That a couple of react clones happen to also use jsx doesn't change this.

It's not framework-specific. General-purpose tools like typescript-eslint generally target the general syntaxes recognized by TypeScript, but not language-specific ones like Ember's.

Ember's is not language specific, if anything it is more generalized JS related than JSX. We built over a draft proposal for content-tag and I'm hoping we can push that through to standardization. Even if we don't get it standardized at the language level, content-tag would provide angular, vue, svelte, ember and any other framework that comes along the ability to quickly reach tool parity with jsx.

@bradzacher
Copy link
Member

I mean JSX has an open spec and is used by non-react frameworks too - eg Vue allows you to use JSX instead of its template syntax.

Regardless we're off topic here.

The fact of the matter is that tsx is a blessed extension in TS and gts is not. And what I said stands true, to my knowledge.
I used shorthand before so I will be verbose to remove any ambiguity:

Ember is the first framework that defines its own language syntax that is not supported by TS out of the box and that chooses to lint that syntax by coercing the code to TS syntax instead of defining their own AST or using pre-processors.

Existing examples:

  • Vue templates are handled by both a custom parser AND a pre-processor. The parser produces a custom Vue template AST for linting and the preprocessor simultaneously extracts the <script> tag contents for linting.
  • Angular templates are handled by a custom parser that produces a custom AST for linting.
  • markdown fences are extracted using a pre-processor
  • graphql fragments are usually parsed out of the JS/TS AST by selecting relevant template literals and running a parser over the text. I think there are some cases of using a pre-processor to extract them as well.
    • graphql files are handled by a custom parser.

There are a lot of examples of other languages and how they work but ember's direction, from what I know, is a first of its kind. Not saying that's a bad thing - it's just an important point in framing the problem.
The issues that you are slash will face will be unique to ember. Eg no other framework has encountered this issue of wanting the eslint-recommened set on a custom extension because their custom extensions are not TS code.

Regardless the solution is clear. Ember users should migrate to flat configs should they want to use our eslint-recommended config on gts files.

@runspired
Copy link

runspired commented Mar 7, 2024

@bradzacher Your response makes me think you don't quite understand the ask here, but I agree at this point this is just an interesting side-tangent as we've been encouraging most Ember apps to migrate to flat configs anyway so its all a moot point for this tool.

But! In the interest of discussion, I really would like to press the point that tools that build-in support for jsx but make it hard for other languages and extensions are really just throttling the ecosystem in favor of one framework's paradigms.

Like the frameworks you mentions above, Ember's templates are also handled by custom parser and pre-processor, and we do convert it all to an AST.

Vue, Angular, and Svelte's current approach of using script tags to get around limitations hard-coded into tooling is clever, but also far more limiting and greatly reduces the dx potential they could have (it also makes many things in tooling even harder to achieve, we actually chose the content-tag approach in part because its one of the easiest paths to comprehensive tooling support).

Ember isn't asking TS or typescript-eslint to parse and understand what is inside of content-tags. We're asking for far less than jsx asks, we were merely hoping we could get this tool to understand that sometimes js/ts files have a different extension than one of these hardcoded "pre-blessed" extensions.

ember's direction, from what I know, is a first of its kind

We usually are the first framework to move on new ideas, but this one is built on a long legacy. In fact this was one of the original motivating factors for tagged templates in the language (and where the template-tag name originated from before we settled on content-tag). E.g.

let X = <template>My Thing</template>;

Is mostly analogous to:

let X = template`My Thing`;

@bradzacher
Copy link
Member

we were merely hoping we could get this tool to understand that sometimes js/ts files have a different extension than one of these hardcoded "pre-blessed" extensions

We do that already - none of our rules assume that a file must have a .[cm]?tsx? extension. And we have parserOptions.extraFileExtensions for configuring type-aware linting to work on custom extensions.

However we have to make the assumption for configs -- I mentioned above why we need to keep the extensions defined.

For legacy configs there's no mechanism to create a "customisable" shareable config with flat configs. The user doesn't import them - eslint does - so there's no interaction point to allow the user to customise things. The configs are fixed to whatever they are as defined in the package.

So if we wanted to provide a solution we'd need to export the config without any file filters to allow the user to reference that raw config and place it in an extends wherever they choose.
Though I just don't see why we'd create that config and export it so that these new users can onboard onto eslint onto the legacy config when that config style is now the deprecated way of doing things.

If we instead focus on the flat config usecase as I mentioned above there is a working solution in the form of an easy spread that allows users to override the default extensions. eslint-plugin-ember could even do this for the user as part of their recommended flat config.

patricklx added a commit to patricklx/eslint-plugin-ember that referenced this issue Mar 11, 2024
…or ts/gts

but they only to that for known extensions, therefore we need to reapply them in our
recommended config
see issue typescript-eslint/typescript-eslint#8607
patricklx added a commit to patricklx/eslint-plugin-ember that referenced this issue Mar 11, 2024
…or ts/gts

but they only to that for known extensions, therefore we need to reapply them in our
recommended config
see issue typescript-eslint/typescript-eslint#8607
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin package: typescript-eslint Issues related to the typescript-eslint package wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants