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

fix: remove unused --save option for set resolution command #5868

Conversation

akwodkiewicz
Copy link
Contributor

@akwodkiewicz akwodkiewicz commented Oct 25, 2023

The option was never implemented in berry, but it's mentioned in the docs as well as in the CLI output.

I don't have the expertise to actually implement the flag, so the second best thing to do here is to remove it, so that people are no longer mislead.

What's the problem this PR addresses?

Mitigates #2202 (actual resolution should be implementing the flag).

How did you fix it?

Removed unnecessary flag.

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.

The option was never implemented in berry, but it's mentioned in the docs as well
as in the CLI output.

See yarnpkg#2202

I don't have the expertise to actually implement the flag, so the second best thing to do here
is to remove it, so that people are no longer mislead.
@akwodkiewicz akwodkiewicz force-pushed the fix/remove-save-option-from-set-resolution branch from aa5a616 to 7fb65d9 Compare November 1, 2023 22:14
@akwodkiewicz
Copy link
Contributor Author

I am not completely sure if there should be some other packages bumped as well. Is upgrading only @yarnpkg/plugin-essentials ok, or should I upgrade also all/some other packages as well?

@clemyan
Copy link
Member

clemyan commented Nov 2, 2023

You need to bump @yarnpkg/cli too because the default plugins are bundled into it

@akwodkiewicz
Copy link
Contributor Author

akwodkiewicz commented Nov 2, 2023

@clemyan, what about ~15 packages that depend on @yarnpkg/cli? Do they have to be bumped as well?

Depending on your answer I'll have to either decline or bump those packages, because
f911b7f is only bumping @yarnpkg/cli and leaving the rest as "undecided".

Dependencies of @yarnpkg/cli are not bumped or declined here, waiting
for more info in the discussion on yarnpkg#5868

Co-Authored-By: Clement Yan <[email protected]>
@clemyan
Copy link
Member

clemyan commented Nov 2, 2023

what about ~15 packages that depend on @yarnpkg/cli? Do they have to be bumped as well?

You can leave those as declined.

The reason you need to bump @yarnpkg/cli is because the code for all default plugins are included and bundled into the @yarnpkg/cli. So in order for users to see your changes, a new version of the CLI needs to be released.

Most other workspaces only have dev dependency on @yarnpkg/cli. The only workspaces that have a prod dependency on it are @yarnpkg/doctor and @yarnpkg/builder, but neither are affected by this change.

The only workspaces that have a non-dev dependency on `@yarnpkg/cli`
are `@yarnpkg/doctor` and `@yarnpkg/builder` but neither of these is
affected by the change in `SetResolutionCommand`.

Co-Authored-By: Clement Yan <[email protected]>
@akwodkiewicz
Copy link
Contributor Author

Awesome, thank you very much for the thorough explanation 👍

@arcanis arcanis merged commit 0ee6213 into yarnpkg:master Nov 2, 2023
24 checks passed
@akwodkiewicz akwodkiewicz deleted the fix/remove-save-option-from-set-resolution branch November 2, 2023 08:51
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.

4 participants