Skip to content

fix: correctly install implicit nested types dependencies #6800

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spanishpear
Copy link
Contributor

@spanishpear spanishpear commented May 17, 2025

What's the problem this PR addresses?

Closes #6442

By default - yarn will automatically add @types/foo for a package, if the package
has a peer dependency on foo. This happens in the normalizePackage core,
and exists as packages often don't specify the correct peer dependencies.

However, given a scenario where:

"dependencies": {
    "peer-deps-implicit-types-conflict": "1.0.0",
    "@types/no-deps": "1.0.0",
}

where peer-deps-implicit-types-conflict has a dependency on @types/no-deps:2.0.0,

  "dependencies": {
    "@types/no-deps": "2.0.0"
  },
  "peerDependencies": {
    "no-deps": "1.0.0"
  }

yarn will only install @types/no-deps:1.0.0 - even though @types/no-deps: 2.0.0 is specified
as a dependency!

How did you fix it?

The fix is rather trivial and thanks to the helpful comment here - #6442 (comment) - simply don't install the automatic @types/foo for a package that has foo as a peer-dependency, if the package already specifies @types/foo as a dependency.

There is already logic for this behavior when it comes to @types/ already existing in peerDependencies or peerDependenciesMeta - this PR just extends that to dependencies as well.
...

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you can see the impact of the fix :D All of these packages specified @types/react as dependency but yarn added an automatic @types/react when it didn't need to

Copy link
Member

Choose a reason for hiding this comment

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

🤔 Can't tell if these being deps rather than peers is correct or not after a cursory look but this change is correct regardless -- if it is problematic then those are bugs that should be fixed upstream.

@spanishpear spanishpear force-pushed the spanishpear/6442-nested-deps branch from 591a4d6 to dbdd8ca Compare May 17, 2025 11:52
@spanishpear
Copy link
Contributor Author

spanishpear commented May 17, 2025

Any opinions on what changeset this should be?

Strictly speaking, it could be considered a breaking change in behavior, but IMO it's is more accurately a minor? 🤔

The failing tests look like flakes unrelated to the changeset.

@clemyan
Copy link
Member

clemyan commented May 21, 2025

I'd consider this a bugfix so patch should suffice

@spanishpear spanishpear force-pushed the spanishpear/6442-nested-deps branch from dbdd8ca to ef47d3b Compare May 28, 2025 09:17
@spanishpear
Copy link
Contributor Author

@clemyan I've added a patch changeset following the interactive tool - let me know if you want any further changes! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?]: yarn with node-linker does not correctly install nested dependencies
2 participants