Bug: TypeScript type error in generated dts files

danielrentz commented 1 month ago

Which packages are affected?

[X] @eslint/compat
[ ] @eslint/config-array
[ ] @eslint/core
[ ] @eslint/migrate-config
[ ] @eslint/object-schema

Environment

Node version: 20 npm version: ESLint version: 9.9.0 Operating System: Win11

What did you do?

using eslint plugins in TS wrapper files
upgrading @stylistic/eslint-plugin from 2.3 to 2.4

A weird side effect in package installation starts to give me a type error from THIS package after upgrading @stylistic/eslint-plugin. See below for details.

What did you expect to happen?

My TS code compiles.

What actually happened?

After upgrading @stylistic/eslint-plugin, I get a type error from THIS package.

node_modules/@eslint/compat/dist/esm/index.d.ts:4:63 - error TS2694: Namespace '"node_modules/@types/eslint/index".Rule' has no exported member 'OldStyleRule'.

4 export type FixupLegacyRuleDefinition = import("eslint").Rule.OldStyleRule;
                                                                ~~~~~~~~~~~~

Found 1 error in node_modules/@eslint/compat/dist/esm/index.d.ts:4

The typedef

/** @typedef {import("eslint").Rule.RuleModule} FixupRuleDefinition */
/** @typedef {FixupRuleDefinition["create"]} FixupLegacyRuleDefinition */

in https://github.com/eslint/rewrite/blob/main/packages/compat/src/fixup-rules.js#L11-L12 has been compiled to

export type FixupRuleDefinition = import("eslint").Rule.RuleModule;
export type FixupLegacyRuleDefinition = import("eslint").Rule.OldStyleRule;

in node_modules/@eslint/compat/dist/esm/index.d.ts, i.e. the type lookup expression FixupRuleDefinition["create"] has been resolved to Rule.OldStyleRule;.

However, the type OldStyleRule exists in @types/eslint v8 only but not in v9.

This seems to be a side effect of the @stylistic/eslint-plugin upgrade mentioned above. With @stylistic/eslint-plugin v2.3 I see

> yarn why @types/eslint
├─ @stylistic/eslint-plugin@npm:2.3.0 [3613d]
│  └─ @types/eslint@npm:8.56.11 (via npm:^8.56.10)
│
└─ @types/eslint__js@npm:8.42.3
   └─ @types/eslint@npm:9.6.0 (via npm:*)

but with v2.4 and above I see

> yarn why @types/eslint
├─ @stylistic/eslint-plugin@npm:2.4.0 [3613d]
│  └─ @types/eslint@npm:9.6.0 (via npm:^9.6.0)
│
└─ @types/eslint__js@npm:8.42.3
   └─ @types/eslint@npm:9.6.0 (via npm:*)

i.e. there is no more @types/eslint v8 anymore in node_modules .

Link to Minimal Reproducible Example

-

Participation

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

Additional comments

No response

fasttime commented 1 month ago

Thanks for the issue @danielrentz. I can see that the latest version of @eslint/compat@1.1.1 is still using an obsolete type definition of Rule.OldStyleRule (in dist/esm/index.ts and dist/cjs/index.ts). This should be fixed in the next release, because @types/eslint was already updated to v9.

That said, it's difficult to say if that update will "just work" for you without a repro to look into. It sounds like your project is relying on a version of @types/eslint that gets installed by a third-party dependency (@stylistic/eslint-plugin) which is possibly not the best setup.

danielrentz commented 1 month ago

Well, my project is not relying on @types/eslint directly, but just uses two packages bringing it into scope (@stylistic/eslint-plugin and @types/eslint__js). Maybe this project should do so too?

fasttime commented 1 month ago

Well, my project is not relying on @types/eslint directly, but just uses two packages bringing it into scope (@stylistic/eslint-plugin and @types/eslint__js). Maybe this project should do so too?

Sorry, are you suggesting that this project (@eslint/compat) should include @stylistic/eslint-plugin and @types/eslint__js as dependencies?

danielrentz commented 1 month ago

No no no, I meant @types/eslint :)

fasttime commented 1 month ago

No no no, I meant @types/eslint :)

* https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/eslint__js/package.json#L9

* https://github.com/eslint-stylistic/eslint-stylistic/blob/main/packages/eslint-plugin/package.json#L49

Ah okay, thanks. Actually the next release of @eslint/compat should work well with both @types/eslint v8 and v9, but I can see why explicitly including it as a dependency in package.json would be useful. I'd say feel free to open a change request if you have a clear proposal so it's easier to discuss it with the rest of the team.

nzakas commented 1 month ago

@fasttime what "next release" are you referring to? There isn't any release pending. Or are you suggesting that some changes have to be made?

@danielrentz the next here is for you to create a repro case, either a StackBlitz or a GitHub repo where we can easily see the behavior you're describing. Without that, it's difficult to really understand what the solution is.

alecmev commented 1 month ago

Minimal repro: https://stackblitz.com/edit/eslint-rewrite-repro-104 Simply run tsc 😉

fasttime commented 1 month ago

@nzakas I didn't consider that #94 would cause a change in the compiled code of @eslint/compat because of the updated @types/eslint dev dependency. In afterthought that PR should have been tagged as feat: rather than chore: and now we would have a pending release. I guess we could trigger release-please with an empty commit since there is nothing else to deploy.

fasttime commented 1 month ago

I'd be slightly in favor of promoting @types/eslint from devDependencies to dependencies in @eslint/compat. This would only benefit TypeScript users who didn't install @types/eslint by themselves, but to a very low cost. Since @eslint/compat is mostly used as a dev dependency itself, there should be no overhead in production for most projects.

nzakas commented 1 month ago

@alecmev thanks!

@fasttime I'm not a fan of installing type-only packages as dependencies, as that doesn't make clear the relationship between the packages, and it could lead to conflicting version issues if someone does have @types/eslint installed. I'm not sure what the right answer for this is yet.

FWIW, empty commits won't trigger release-please in a monorepo because it uses the changed files to determine which package the change belongs to. We'd need to make a change to an actual file.

alecmev commented 4 weeks ago

What about a peer dependency? You can explore prior art here. @types/react-redux is a popular example.

nzakas commented 3 weeks ago

@JoshuaKGoldberg do you have any insights on how to handle a situation like this? The suggestion is to add @types/eslint as a dependency or peer dependency, but that seems like it's not correctly defining the relationship between the package and the types.

nzakas commented 1 week ago

Ping @JoshuaKGoldberg

JoshuaKGoldberg commented 1 week ago

Sorry! Missed this. Yes, it's unfortunate - there's no way to delineate a runtime end-user dependency from a types end-user dependency. A common strategy is to declare the @types/* packages under devDependencies + peerDependencies and use peerDependenciesMeta to declare them as optional.

We do this in typescript-eslint for the typescript dependency in several user-facing packages. For example, in @typescript-eslint/eslint-plugin:

  "peerDependencies": {
    "@typescript-eslint/parser": "^8.0.0 || ^8.0.0-alpha.0",
    "eslint": "^8.57.0 || ^9.0.0"
  },
  "peerDependenciesMeta": {
    "typescript": {
      "optional": true
    }
  },

I'd lean towards that.

eslint / rewrite