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: TypeScript type error in generated dts files #104

Closed
1 of 6 tasks
danielrentz opened this issue Aug 13, 2024 · 16 comments · Fixed by #106
Closed
1 of 6 tasks

Bug: TypeScript type error in generated dts files #104

danielrentz opened this issue Aug 13, 2024 · 16 comments · Fixed by #106
Labels
accepted bug Something isn't working repro:yes Issues with a reproducible example

Comments

@danielrentz
Copy link
Contributor

Which packages are affected?

  • @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

@danielrentz danielrentz added bug Something isn't working repro:needed This issue should include a reproducible example labels Aug 13, 2024
@fasttime
Copy link
Member

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

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

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?

@fasttime
Copy link
Member

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

nzakas commented Aug 13, 2024

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

alecmev commented Aug 15, 2024

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

@fasttime
Copy link
Member

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

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

nzakas commented Aug 15, 2024

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

alecmev commented Aug 18, 2024

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

@nzakas
Copy link
Member

nzakas commented Aug 20, 2024

@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.

sndrs added a commit to guardian/csnx that referenced this issue Sep 3, 2024
…onfig

Installing it alongside `@eslint/compat` creates a types
clash in `node_modules`.

It will be re-added in a future release.

eslint/rewrite#104.
@nzakas
Copy link
Member

nzakas commented Sep 3, 2024

Ping @JoshuaKGoldberg

@JoshuaKGoldberg
Copy link
Contributor

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.

@fasttime fasttime added repro:yes Issues with a reproducible example and removed repro:needed This issue should include a reproducible example labels Sep 3, 2024
@nzakas
Copy link
Member

nzakas commented Sep 16, 2024

Thanks @JoshuaKGoldberg, that makes sense. @danielrentz do you want to update your PR accordingly?

@danielrentz
Copy link
Contributor Author

@nzakas done

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 repro:yes Issues with a reproducible example
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants