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: @eslint/compat types incompatible with both typescript-eslint and @types/eslint #25

Closed
1 of 4 tasks
karlhorky opened this issue May 19, 2024 · 10 comments · Fixed by #28
Closed
1 of 4 tasks
Labels
accepted bug Something isn't working

Comments

@karlhorky
Copy link

karlhorky commented May 19, 2024

Originally reported in typescript-eslint/typescript-eslint#9115

Which packages are affected?

  • @eslint/compat
  • @eslint/config-array
  • @eslint/object-schema

Environment

Node version: v20.13.1
npm version: 10.5.2
ESLint version: 9.3.0
Operating System: macOS Sonoma 14.5 (23F79)

What did you do?

I used fixupPluginRules() from @eslint/compat along with the tseslint.config() config definition function from typescript-eslint and received a type mismatch:

// @ts-check

import { fixupPluginRules } from '@eslint/compat';
import react from 'eslint-plugin-react';
import tseslint from 'typescript-eslint';

const configArray = tseslint.config({
  plugins: {
    react: fixupPluginRules(react), // 💥 Type 'FixupPluginDefinition' is not assignable to type 'Omit<Plugin, "configs">'.
                                    // ... (see full error below under "What actually happened?")
  },
});

export default configArray;

See full error below under "What actually happened?".

I also tried the Linter.FlatConfig[] type from eslint itself:

// @ts-check

import { fixupPluginRules } from '@eslint/compat';
import react from 'eslint-plugin-react';

/** @type {import('eslint').Linter.FlatConfig[]} */
const configArray = [
  {
    plugins: {
      react: fixupPluginRules(react), // 💥 Type 'FixupPluginDefinition' is not assignable to type 'Plugin'.
                                      // ... (see full error below under "What actually happened?")
    },
  },
];

export default configArray;

See full error below under "What actually happened?".

My package.json:

{
  "type": "module",
  "devDependencies": {
    "@eslint/compat": "1.0.1",
    "@eslint/js": "9.3.0",
    "@types/eslint": "8.56.10",
    "eslint": "9.3.0",
    "eslint-plugin-react": "7.34.1",
    "typescript": "5.4.5",
    "typescript-eslint": "8.0.0-alpha.14"
  },
  "packageManager": "pnpm@9.1.1+sha256.9551e803dcb7a1839fdf5416153a844060c7bce013218ce823410532504ac10b"
}

What did you expect to happen?

The types from @eslint/compat should be compatible with both:

  1. tseslint.config() config definition function from typescript-eslint
  2. Linter.FlatConfig[] type from @types/eslint

What actually happened?

The types were reported as incompatible with both:

1. typescript-eslint

tseslint.config() config definition function from typescript-eslint

Type 'FixupPluginDefinition' is not assignable to type 'Omit<Plugin, "configs">'.
  Types of property 'rules' are incompatible.
    Type 'Record<string, FixupRuleDefinition> | undefined' is not assignable to type 'Record<string, LooseRuleDefinition> | undefined'.
      Type 'Record<string, FixupRuleDefinition>' is not assignable to type 'Record<string, LooseRuleDefinition>'.
        'string' index signatures are incompatible.
          Type 'FixupRuleDefinition' is not assignable to type 'LooseRuleDefinition'. ts(2322)

(the types of both typescript-eslint@7.9.0 and typescript-eslint@8.0.0-alpha.14 result in the same problem, I chose v8 alpha for its support of ESLint 9)

2. @types/eslint

Linter.FlatConfig[] type from @types/eslint

Type 'FixupPluginDefinition' is not assignable to type 'Plugin'.
  Types of property 'rules' are incompatible.
    Type 'Record<string, FixupRuleDefinition> | undefined' is not assignable to type 'Record<string, RuleModule | ((context: RuleContext) => RuleListener)> | undefined'.
      Type 'Record<string, FixupRuleDefinition>' is not assignable to type 'Record<string, RuleModule | ((context: RuleContext) => RuleListener)>'.
        'string' index signatures are incompatible.
          Type 'FixupRuleDefinition' is not assignable to type 'RuleModule | ((context: RuleContext) => RuleListener)'. ts(2322)

This is similar to the outcome with fixupConfigRules reported in #24

Cause

Apparently the types of @eslint/compat are using loose, problematic types like Object, as @bradzacher mentions here:

These appear to have been added as part of #5 here:

Link to Minimal Reproducible Example

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

No response

@karlhorky
Copy link
Author

karlhorky commented May 19, 2024

Also, the argument type of the first argument of fixupPluginRules() doesn't match either of FlatConfig.Plugin from typescript-eslint or ESLint.Plugin from @types/eslint:

import { fixupPluginRules } from '@eslint/compat';

// typescript-eslint
const typescriptEslintPlugin =
  /** @type {import('@typescript-eslint/utils/ts-eslint').FlatConfig.Plugin} */ ({});
fixupPluginRules(typescriptEslintPlugin); // 💥 Argument of type 'Plugin' is not assignable to parameter of type 'FixupPluginDefinition'.
                                          // ... (see full error below)

// @types/eslint
const eslintPlugin = /** @type {import('eslint').ESLint.Plugin} */ ({});
fixupPluginRules(eslintPlugin); // 💥 Argument of type 'Plugin' is not assignable to parameter of type 'FixupPluginDefinition'.
                                // ... (see full error below)

I also added this as a file fixupPluginRules-argument-type-mismatch.js in the reproduction repo:

Errors:

typescript-eslint

Argument of type 'Plugin' is not assignable to parameter of type 'FixupPluginDefinition'.
  Types of property 'rules' are incompatible.
    Type 'Record<string, LooseRuleDefinition> | undefined' is not assignable to type 'Record<string, FixupRuleDefinition> | undefined'.
      Type 'Record<string, LooseRuleDefinition>' is not assignable to type 'Record<string, FixupRuleDefinition>'.
        'string' index signatures are incompatible.
          Type 'LooseRuleDefinition' is not assignable to type 'FixupRuleDefinition'.
            Type 'LooseRuleCreateFunction' is not assignable to type 'FixupRuleDefinition'.ts(2345)

@types/eslint

Argument of type 'Plugin' is not assignable to parameter of type 'FixupPluginDefinition'.
  Types of property 'rules' are incompatible.
    Type 'Record<string, RuleModule | ((context: RuleContext) => RuleListener)> | undefined' is not assignable to type 'Record<string, FixupRuleDefinition> | undefined'.
      Type 'Record<string, RuleModule | ((context: RuleContext) => RuleListener)>' is not assignable to type 'Record<string, FixupRuleDefinition>'.
        'string' index signatures are incompatible.
          Type 'RuleModule | ((context: RuleContext) => RuleListener)' is not assignable to type 'FixupRuleDefinition'.
            Type '(context: RuleContext) => RuleListener' is not assignable to type 'FixupRuleDefinition'.ts(2345)

@karlhorky karlhorky changed the title Bug: @eslint/compat types incompatible with both typescript-eslint and eslint types Bug: @eslint/compat types incompatible with both typescript-eslint and @types/eslint May 19, 2024
@nzakas
Copy link
Member

nzakas commented May 20, 2024

Thanks for the report. Unfortunately, the team doesn't have a lot of experience with TypeScript (this is one of the first packages we've published with type definitions), so I'm not surprised that there are conflicts.

I (naively) thought that by defining interfaces, then anything matching that interface would be accepted.

Also unfortunately, it seems that others have gone ahead and created type definitions for ESLint that we weren't involved in (such as @types/eslint), which is going to make it difficult to reconcile.

As it seems like you have a great deal of insight into this problem, would you be willing to put together a proposal for how to address this?

@nzakas nzakas moved this from Needs Triage to Triaging in Triage May 20, 2024
@karlhorky
Copy link
Author

karlhorky commented May 20, 2024

Thanks for the candid feedback and openness to make a change here!

For additional feedback regarding "the future of TS types in official ESLint projects" I would like to defer to community members who have worked on the @types/eslint and typescript-eslint projects:


My first gut feel - given the years of existing, detailed work in A) @types/eslint eg. index.d.ts,
B) typescript-eslint - lots of files there, eg packages/utils/src/ts-eslint/Rule.ts and C) related packages like @types/estree - would be to:

  1. Adjust the types of @eslint/compat to consume these types
  2. Create pull requests for DefinitelyTyped if there are inaccuracies in the types

In future, if the ESLint project has interest, then eslint/eslint could become the owner of the types, and @types/eslint could be deprecated.

For example, below would be a first quick sketch (generated code, not tested to be accurate or compatible types) of using the DefinitelyTyped packages @types/eslint and @types/estree to build the @eslint/compat types:

packages/compat/src/types.ts

/**
 * @filedescription Types for backcompat package.
 */

import type { Rule, Linter, Scope, SourceCode } from 'eslint';
import { Node as ESTreeNode } from 'estree';

/*
 * NOTE: These are minimal type definitions to help avoid errors in the
 * backcompat package. They are not intended to be complete and should be
 * replaced when the actual types are available.
 */

export type FixupLegacyRuleDefinition = (
  context: Rule.RuleContext,
) => Rule.RuleListener;

export interface FixupRuleDefinition {
  meta?: Rule.RuleMetaData;
  create(context: Rule.RuleContext): Rule.RuleListener;
}

export interface FixupPluginDefinition {
  meta?: object;
  rules?: Record<string, FixupRuleDefinition>;
  configs?: Record<string, Linter.Config>;
  processors?: Record<string, Linter.Processor>;
}

export interface FixupConfig {
  files?: string[];
  ignores?: string[];
  name?: string;
  languageOptions?: {
    ecmaVersion?: number | 'latest';
    sourceType?: 'script' | 'module';
    globals?: Record<string, boolean | 'readonly' | 'writable' | 'writeable'>;
    parser?: string;
    parserOptions?: Linter.ParserOptions;
  };
  linterOptions?: {
    noInlineConfig?: boolean;
    reportUnusedDisableDirectives?: boolean | Linter.RuleLevel;
  };
  processor?: string | Linter.Processor;
  plugins?: Record<string, FixupPluginDefinition>;
  rules?: Record<string, Linter.RuleEntry>;
}

export type FixupConfigArray = FixupConfig[];

@nzakas What are your thoughts on this idea of consuming the DefinitelyTyped community packages like this?

@nzakas
Copy link
Member

nzakas commented May 21, 2024

What would be the benefit of that vs. just using the types from @types/eslint directly?

@karlhorky
Copy link
Author

karlhorky commented May 21, 2024

using the types from @types/eslint directly

By "using the types", do you mean copying the types instead of importing them?

In that case, the benefit of importing + adding a dependency vs. copying the types would be that any incompatible updates to types on either side (either @types/node or eslint/rewrite) would cause visible failures in the CI of eslint/rewrite.

There would be ways around this, eg:

  1. Copying the types
  2. Also adding an additional "layer" of type tests, which would depend on the types in @types/eslint and test against them

But these options would lead to more work, maintenance and indirection in the code.

@karlhorky
Copy link
Author

karlhorky commented May 21, 2024

Or maybe I misunderstood: maybe by "using the types" you mean importing more of the types that are already created, effectively removing most or all new types in packages/compat/src/types.ts in eslint/rewrite

(any new types which are needed could also be contributed back to @types/eslint on DefinitelyTyped)

That seems to me like an even better approach, if it is agreeable! (as it adds back to the community efforts that are going on in those packages)

@nzakas
Copy link
Member

nzakas commented May 24, 2024

Or maybe I misunderstood: maybe by "using the types" you mean importing more of the types that are already created, effectively removing most or all new types in packages/compat/src/types.ts in eslint/rewrite

Yes, exactly. As I mentioned, we on the team aren't super familiar with TypeScript and I was relying on my experience of using interfaces in other languages when putting this together. If we can use the types of @types/eslint directly, I'm fine with that.

What I don't want to do is end up maintaining @types/eslint. I'm not super thrilled that we're being held to types that people wrote who aren't on the team and didn't attempt to collaborate with us on. Ultimately, when we do our core rewrite, we'll end up with our type definitions that we'll maintain along with the code.

@karlhorky
Copy link
Author

karlhorky commented May 24, 2024

If we can use the types of @types/eslint directly, I'm fine with that.

Seems pretty good to me - rely on the things that are already available.

What I don't want to do is end up maintaining @types/eslint

Ultimately, when we do our core rewrite, we'll end up with our type definitions that we'll maintain along with the code.

Understood, taking over a large project created and maintained by others is not easy.

Two notes here:

  1. The DefinitelyTyped types (all of the packages prefixed with @types/) are usually high quality and represent the visible runtime type information of the library. So you may be surprised by the quality.
  2. If ESLint wants to take over publishing the types, great! However, what would be good for the ecosystem is if there's only 1 source to get the full types from though (either @types/eslint or eslint). What would be problematic would be if eslint were to publish only a subset of the types in @types/eslint, making the community have to install 2 packages to get complete types. Gut feel says this would be a messy overlap and would lead to high likelihood of type incompatibilities. So my suggestion here would be for eslint to take over all of the complete types of @types/eslint (and maybe also other related types packages) and probably refrain from publishing any TS types from eslint itself until that has been done. During the migration, TS types could still be used internally in eslint (but not published), and slowly copied over from @types/eslint, with tests to make sure that they don't diverge from new versions of @types/eslint.

That's my take on this, maybe a DefinitelyTyped or TS or typescript-eslint community member would want to chime in as well though - I may be missing an opportunity to do this migration of types from @types/eslint to eslint in a cleaner or simpler way, while maintaining low disruption and breakage for the ecosystem.

@jakebailey
Copy link

jakebailey commented May 24, 2024

Just to be clear, I have not personally worked on @types/eslint, the TS team just has a rotation of people who look at DT, and some specific people who tend to work on DT tooling.

Just for the immediate feedback, I would definitely be trying to get the types here in better shape. Using Object is definitely a bad time, it's just not the type you might think it is. ts-eslint does have rules that try and point those out, but it'd end up being better as {}, Record<string, ...>, object, etc.

I would probably consider depending on @types/eslint here just to prevent breakage, though; I'm not 100% certain what is and isn't compatible between @types/eslint and typescript-eslint, but I assume the former can be used for the latter and be okay. Alternatively, you can create test projects in this repo which serve just to link to the packages and verify that they compile against other types.

As for ESLint itself publishing types, I think that comes down to how breaky you want that to be. Realistically, the best situation is to adopt the types from @types/eslint into ESLint itself so that downstream users don't break, but theoretically if ESLint does another major bump, it could break the types entirely and that would probably be socially fine. I don't have a lot of experience myself in these kinds of transitions. I'm also not sure what is "bad" in @types/eslint, though; they don't look very objectionable to me, mostly boilerplate to deal with AST nodes (maybe goes a little too far into type-only namespace usage).

@nzakas
Copy link
Member

nzakas commented May 27, 2024

Just to be clear, I wasn't suggesting we'd export types from the eslint package. That would be a monumental effort for not enough value.

For the rewritten core, though, we'll definitely be exporting our own types.

nzakas added a commit that referenced this issue May 27, 2024
@github-project-automation github-project-automation bot moved this from Triaging to Complete in Triage May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants