Skip to content

fix: add missing peer dependencies #9744

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 16, 2024
Merged

fix: add missing peer dependencies #9744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/type-utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@
"debug": "^4.3.4",
"ts-api-utils": "^1.3.0"
},
"peerDependencies": {
"eslint": "^8.57.0 || ^9.0.0"
},
"devDependencies": {
"@jest/types": "29.6.3",
"@typescript-eslint/parser": "8.0.1",
Expand Down
3 changes: 3 additions & 0 deletions packages/typescript-eslint/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@
"@typescript-eslint/parser": "8.0.1",
"@typescript-eslint/utils": "8.0.1"
},
"peerDependencies": {
"eslint": "^8.57.0 || ^9.0.0"
},
"devDependencies": {
"@jest/types": "29.6.3",
"downlevel-dts": "*",
Expand Down
5 changes: 5 additions & 0 deletions packages/utils/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@
"rimraf": "*",
"typescript": "*"
},
"peerDependenciesMeta": {
"typescript": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Question] Shouldn't this also be listed in peerDependencies? Even if listing it in peerDependenciesMeta implicitly adds it there, it feels odd to me to make that implicit.

Copy link
Contributor Author

@ethanwu10 ethanwu10 Nov 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied this from type-utils (which is where the missing dep for typescript comes from), which also does not list it as an explicit peerdep

typescript-eslint/packages/type-utils/package.json

Lines 64 to 68 in 42d275c

"peerDependenciesMeta": {
"typescript": {
"optional": true
}
},

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, nice, so this might be an existing issue in that package then? My vote is to fix in both packages.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just came here to say that appearently at least both Yarn and pnpm have opted in the past to have "peerDependenciesMeta without peerDependencies" imply a peer dependency on *. This was done due to some (legacy?) npm behavior regarding peer dependencies, making package authors hesistant to list them. See pnpm/pnpm#2128.

But it's probably a better practice to explicitly list them; and given the age of the referenced issue probably safe enough as well.

Copy link
Member

@bradzacher bradzacher Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we used peerDependenciesMeta without peerDependencies was because there were versions of npm that did not support peerDependenciesMeta. This meant that when we used an explicit peerDependencies then users would be spammed with warnings saying they didn't have TS installed. This was a problem as some big packages included our plugins by default but only used them if the user opted in to TS which caused non-TS users to get spammed with missing peer dep errors.

A this point we could probably also include the explicit peerDependencies on TS but also there's no harm in not including it and having it inferred to ensure that nobody gets spammed with logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know if npm also shares this behavior of peerDependenciesMeta implying peerDependencies? From a cursory search of the npm sources it seems to me like it doesn't: https://github.com/npm/cli/blob/75a3f1228865f426d8790be27f1258e501f2c450/workspaces/arborist/lib/node.js#L859-L871

Anyways, what do we think of merging this PR first and moving the discussion about whether or not to list an explicit peerdep to a different issue? We don't need to block fixing users' warnings on figuring out the proper manifest declarations 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👍 on that. If nobody else weighs in with a contrary opinion by our weekly release midday Monday EST then we can just merge as-is.

Now that you mention it, I do like the idea of having one release just adding peerDependenciesMeta, then a subsequent release adding peerDependencies on top. Just in case there's some bizarre edge case bug with the latter.

"optional": true
}
},
"funding": {
"type": "opencollective",
"url": "https://opencollective.com/typescript-eslint"
Expand Down
7 changes: 7 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5797,6 +5797,8 @@ __metadata:
rimraf: "*"
ts-api-utils: ^1.3.0
typescript: "*"
peerDependencies:
eslint: ^8.57.0 || ^9.0.0
peerDependenciesMeta:
typescript:
optional: true
Expand Down Expand Up @@ -5986,6 +5988,9 @@ __metadata:
typescript: "*"
peerDependencies:
eslint: ^8.57.0 || ^9.0.0
peerDependenciesMeta:
typescript:
optional: true
languageName: unknown
linkType: soft

Expand Down Expand Up @@ -19469,6 +19474,8 @@ __metadata:
prettier: ^3.2.5
rimraf: "*"
typescript: "*"
peerDependencies:
eslint: ^8.57.0 || ^9.0.0
peerDependenciesMeta:
typescript:
optional: true
Expand Down
Loading