typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.8k

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

bradzacher merged 1 commit intotypescript-eslint:mainfromethanwu10:main

Nov 16, 2024

Merged

fix: add missing peer dependencies#9744

bradzacher merged 1 commit intotypescript-eslint:mainfromethanwu10:main

Nov 16, 2024

Conversation

Copy link

Contributor

ethanwu10 commentedAug 7, 2024•
edited by JoshuaKGoldberg
Loading

PR Checklist

Addresses an existing open issue:fixesBug: Missing peer dependencies #9224
That issue was marked asaccepting prs
Steps inContributing were taken

Overview

Ensure all transitive peer dependencies are properly listed. This fixes peer-requirements warnings thrown by yarn.

fix: add missing peer dependencies

dd06088

Ensure all transitive peer dependencies are properly listed. This fixespeer-requirements warnings thrown by yarn.

Copy link

Contributor

typescript-eslintbot commentedAug 7, 2024

Thanks for the PR,@ethanwu10!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlifybot commentedAug 7, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`dd06088`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/66b2d565dee66b0007510772
😎 Deploy Preview	https://deploy-preview-9744--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 95 (no change from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 90 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to yourNetlify site configuration.

Copy link

nx-cloudbot commentedAug 7, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitdd06088. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 fromNxCloud.

Copy link

Contributor

rtritto commentedAug 27, 2024

Any update?

FYI@JoshuaKGoldberg

Copy link

Member

bradzacher commentedAug 27, 2024•
edited
Loading

@rtritto The issue has not been marked as accepting PRs.
As a team we have yet to discuss it.

Please do not ping on issues or tag maintainers in comments.
We asVOLUNTEER maintainers will get to this when we have the bandwidth.

JoshuaKGoldberg marked this pull request as draft

August 27, 2024 14:09

Copy link

Contributor

rtritto commentedSep 19, 2024

I have this warnings afteryarn explain peer-requirements:

p4ee77 → ✓ @typescript-eslint/parser@npm:8.6.0 [3223e] doesn't provide @types/supports-color to debug@npm:4.3.6 [1ff4b]p42ec1 → ✓ @typescript-eslint/parser@npm:8.6.0 [3223e] doesn't provide supports-color to debug@npm:4.3.6 [1ff4b]p5cc2b → ✓ @typescript-eslint/type-utils@npm:8.6.0 [8a609] doesn't provide @types/eslint to @typescript-eslint/utils@npm:8.6.0 [7f592] and 1 other dependencyp0f521 → ✓ @typescript-eslint/type-utils@npm:8.6.0 [8a609] doesn't provide @types/supports-color to debug@npm:4.3.6 [1ff4b]p6eab7 → ✘ @typescript-eslint/type-utils@npm:8.6.0 [8a609] doesn't provide eslint to @typescript-eslint/utils@npm:8.6.0 [7f592] and 1 other dependencyp31167 → ✓ @typescript-eslint/type-utils@npm:8.6.0 [8a609] doesn't provide supports-color to debug@npm:4.3.6 [1ff4b]pa4871 → ✓ @typescript-eslint/typescript-estree@npm:8.6.0 [346c1] doesn't provide @types/supports-color to debug@npm:4.3.6 [1ff4b]p66f84 → ✓ @typescript-eslint/typescript-estree@npm:8.6.0 [346c1] doesn't provide supports-color to debug@npm:4.3.6 [1ff4b]p67afe → ✓ @typescript-eslint/typescript-estree@npm:8.6.0 [7f592] doesn't provide @types/supports-color to debug@npm:4.3.6 [1ff4b]p1b301 → ✓ @typescript-eslint/typescript-estree@npm:8.6.0 [7f592] doesn't provide supports-color to debug@npm:4.3.6 [1ff4b]pf4e07 → ✓ @typescript-eslint/utils@npm:8.6.0 [7f592] doesn't provide @types/typescript to @typescript-eslint/typescript-estree@npm:8.6.0 [346c1] and 1 other dependencypb85fd → ✘ @typescript-eslint/utils@npm:8.6.0 [7f592] doesn't provide typescript to @typescript-eslint/typescript-estree@npm:8.6.0 [346c1] and 1 other dependencypa9c59 → ✓ @typescript-eslint/utils@npm:8.6.0 [8a609] doesn't provide @types/typescript to @typescript-eslint/typescript-estree@npm:8.6.0 [346c1] and 1 other dependencypafe94 → ✘ @typescript-eslint/utils@npm:8.6.0 [8a609] doesn't provide typescript to @typescript-eslint/typescript-estree@npm:8.6.0 [346c1] and 1 other dependency

I found a workaround (with yarn berry) to this issue:

# .yarnrc.ymlpackageExtensions:"@typescript-eslint/type-utils@*":dependencies:eslint:^8# don't use v9"@typescript-eslint/utils@*":dependencies:typescript:"*"

There is an article written by author of yarn package manager:Implicit Transitive Peer Dependencies

bradzacher mentioned this pull request

Sep 30, 2024

fix(typescript-eslint): add eslint as peer dependency#10079

Closed

3 tasks

Copy link

Contributor

rtritto commentedOct 18, 2024•
edited
Loading

Alsotypescript-eslint if affected by this issue.

Updated workaround using onlytypescript-eslint:

# .yarnrc.ymlpackageExtensions:"@typescript-eslint/type-utils@*":dependencies:eslint:"*""@typescript-eslint/utils@*":dependencies:typescript:"*"typescript-eslint@*:peerDependencies:eslint:"*"

JoshuaKGoldberg requested changes

Nov 10, 2024

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Thanks for sending this in, and sorry for the wait!

This seems reasonable & straightforward to me. Just one question on theutils package's meta.

packages/utils/package.json

		@@ -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 inpeerDependencies? Even if listing it inpeerDependenciesMeta implicitly adds it there, it feels odd to me to make that implicit.

Copy link

ContributorAuthor

ethanwu10Nov 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 fromtype-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 in42d275c

	"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 withoutpeerDependencies" imply a peer dependency on*. This was done due to some (legacy?) npm behavior regarding peer dependencies, making package authors hesistant to list them. Seepnpm/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

bradzacherNov 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 usedpeerDependenciesMeta withoutpeerDependencies was because there were versions of npm that did not supportpeerDependenciesMeta. This meant that when we used an explicitpeerDependencies 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 explicitpeerDependencies 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

ContributorAuthor

ethanwu10Nov 15, 2024

Choose a reason for hiding this comment

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

Do you know ifnpm also shares this behavior ofpeerDependenciesMeta implyingpeerDependencies? 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

JoshuaKGoldbergNov 15, 2024

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 addingpeerDependenciesMeta, then a subsequent release addingpeerDependencies on top. Just in case there's some bizarre edge case bug with the latter.

JoshuaKGoldberg marked this pull request as ready for review

November 10, 2024 16:48

JoshuaKGoldberg added awaiting response

Issues waiting for a reply from the OP or another party

1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge and removed awaiting responseIssues waiting for a reply from the OP or another party labels

Nov 10, 2024

JoshuaKGoldberg approved these changes

Nov 15, 2024

View reviewed changes

Copy link

Member