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

Conditional dependencies #3575

Merged
merged 19 commits into from
Oct 17, 2021
Merged

Conditional dependencies #3575

merged 19 commits into from
Oct 17, 2021

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Oct 14, 2021

What's the problem this PR addresses?

Fixes #3317

How did you fix it?

I'll write in more details tomorrow, but in a few words:

  • Yarn will only download by default packages relevant to the current system.
  • More packages can be "overfetched" for zero-install purposes via the supportedArchitectures setting.
  • Optional peer dependencies don't have their checksum computed nor stored.
  • The point above isn't a security risk because --check-cache will still validate them, for zero-install users.
  • When stored inside the offline mirror, their paths use cache keys, not checksum keys (since we don't keep the checksum).
  • They will always be unplugged when installed with PnP (the assumption being that they contain native bins anyway).
  • Transitive dependencies of optional native dependencies are always fetched (unless themselves have os/cpu markers).

Note that while it contains breaking changes, I plan to land it as a minor. This is due to multiple factors:

  • Most changes are on the internal API, which aren't used that much and still a little experimental.
  • Most changes in those API are fairly small (extra fields, a few parameters on the cache), shouldn't break much.
  • For a significant segment of our users, this would appear as a bugfix more than an improvement.
  • I don't want to release a 4.0 just for the sake of this one thing.

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.

@arcanis

This comment has been minimized.

@arcanis arcanis force-pushed the mael/optional-native-dependencies branch from 30a3b02 to c2b775c Compare October 15, 2021 14:23
@arcanis arcanis marked this pull request as ready for review October 15, 2021 14:46
Copy link
Member

@paul-soporan paul-soporan left a comment

Choose a reason for hiding this comment

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

Just finished a quick review - I looked at a bit of everything except the tests.

I'll do a more in-depth review (including the tests) once you resolve these comments.

Nit: In the PR description you wrote Optional peer dependencies don't have their checksum computed nor stored., it should be Conditional dependencies.

packages/yarnpkg-core/sources/Cache.ts Outdated Show resolved Hide resolved
packages/plugin-exec/sources/ExecResolver.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/Cache.ts Show resolved Hide resolved
yarn.lock Show resolved Hide resolved
packages/yarnpkg-core/sources/Project.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/structUtils.ts Show resolved Hide resolved
packages/yarnpkg-core/sources/Project.ts Outdated Show resolved Hide resolved
packages/yarnpkg-core/sources/Project.ts Show resolved Hide resolved
packages/yarnpkg-core/sources/Cache.ts Outdated Show resolved Hide resolved
.yarnrc.yml Outdated Show resolved Hide resolved
arcanis and others added 2 commits October 15, 2021 21:19
Co-authored-by: Paul Soporan <paul.soporan@gmail.com>
.yarnrc.yml Outdated Show resolved Hide resolved
arcanis and others added 2 commits October 15, 2021 21:31
Co-authored-by: Paul Soporan <paul.soporan@gmail.com>
yarn.lock Show resolved Hide resolved
@arcanis arcanis merged commit ccb0788 into master Oct 17, 2021
@arcanis arcanis deleted the mael/optional-native-dependencies branch October 17, 2021 16:11
@arcanis arcanis changed the title Optional native dependencies Conditional dependencies Oct 17, 2021
@arcanis
Copy link
Member Author

arcanis commented Oct 17, 2021

I released this change in 3.1.0-rc.11. I plan to let it bake a bit, and if everything looks good I'll aim to release the 3.1 this week.

@merceyz
Copy link
Member

merceyz commented Oct 21, 2021

Reverted the avoidable breaking changes in #3612 so plugin authors shouldn't notice much if they're not using jsInstallUtils

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 downloads irrelevant optional dependencies (e.g. for a different OS)
3 participants