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

Add proposal for new CLI command: yarn upgrade-transitive #54

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

Conversation

magicmark
Copy link

@magicmark magicmark commented Mar 20, 2017

I'm still scoping the possible implementation of this. This is early stage, but I would appreciate any early feedback on this concept!

(yarnpkg/yarn#2543)

@bestander
Copy link
Member

I love the idea to have control over what dependencies Yarn installs transitively.
Not sure if upgrade-interactive will be a good place because average projects that I come around have 500+ dependencies, the UI might be unusable.

What do you think of a separate command for this feature?
Maybe like this

yarn update-transitive react/babel-runtime/[email protected] 
# updates a specific dependency to a version
or
yarn update-transitive */babel-runtime/[email protected] 
# updates all core-js under babel-runtime to a version
or
yarn update-transitive */react-core 
# updates all core-js to latest version within package.json limits

Then it would allow a fine control if you want to patch (or roll back) a deep dependency or many of them

@dfreeman
Copy link

@bestander That sounds exactly like the level of control I keep finding myself wishing for. Would love to see something like that ❤️

@ljharb
Copy link

ljharb commented Mar 23, 2017

Should the * work like globs tho, where ** and * are different?

@bestander
Copy link
Member

bestander commented Mar 23, 2017 via email

@magicmark
Copy link
Author

Separate command sounds entirely reasonable to me.

I'll revise the RFC :)

@Vinnl
Copy link

Vinnl commented May 16, 2017

What would probably also be useful would be a --transitive flag on the yarn outdated command, to find out which transitive dependencies you'd want to update.

@presidenten
Copy link

presidenten commented May 31, 2017

Nice!
I added an issue requesting a similar functionality a few months ago.

A command like yarn update-transitive would do the trick for me as well.
Im crossing my fingers for this rfc!

My previous yarn issue: yarnpkg/yarn#2558

@bestander
Copy link
Member

Ping, does anyone want to send a PR?

@magicmark
Copy link
Author

@bestander I've updated the RFC. Still working on a code implementation (which will also help me answer some of the questions I still have regarding the implementation details in the RFC). Happy if anyone beats me to it, but I am working on it, apologies for the slowness!

@bestander
Copy link
Member

No worries, @magicmark!
Thanks for checking in and the work you do

@sejoker
Copy link

sejoker commented Jun 4, 2017

Correct me if I wrong, but the issue this RFC aims to solve is similar to Selective versions resolutions. If yes I would go with resolutions mechanism as more flexible and obvious how to use. I urge @magicmark take a look and probably join the forces on landing united RFC which address all related issues.

@magicmark
Copy link
Author

magicmark commented Jun 4, 2017

@sejoker Thank you for this info. Looks like it makes sense for the other RFC to contain most of the logic for this as it is more generalised.

There could still be value in this RFC just becoming an interface for adding lines to the package.resolutions, field, regenerating the lockfile and updating node_modules?

@victornoel
Copy link
Contributor

@magicmark I'm the author of the #68 RFC, don't hesitate to make comment there to clarify points that could be helpful to make it coherent with your RFC! Also, I saw the bit about vocabulary, if you think using "transitive dependency" is clearer than "nested dependency", please comment about it there, we could improve the language thus :)

@presidenten
Copy link

As a non native english speaker I put my vote on nested dependency

@bestander
Copy link
Member

It makes sense to be consistent in implementation and vocabulary between this RFC and #68.
Maybe this one could actually piggy-back on #68 implementation?

@magicmark
Copy link
Author

magicmark commented Jun 15, 2017

@bestander sounds good to me. I've given my feedback in #68, I'll await @victornoel:

Actually, there was good reason to discuss that here (so no problem with the hijacking ;), because we could consider that doing: yarn upgrade-interactive '**/some-package' would allow to generate valid package.resolutions from the command line! So I will maybe include it in the RFC too, at least for discussing it!

If this is included in #68, we can drop this?

@bestander
Copy link
Member

@magicmark, we might want to discuss more how upgrade-interactive would work modifying the resolutions described in #68.

@bestander
Copy link
Member

bestander commented Jun 30, 2017

@magicmark sorry for stalling on reviewing this RFC.
I have read it through once more after going through #68 and #71.

It seems like your RFC covers and intends to solve another case that neither of those do.
Here is an example.

We have a structure of packages

A@^1.1.0 -> B@^2.5.0 -> C@^3.1.0

And let's say they got locked in yarn.lock as

After some time all 3 got new versions, e.g. [email protected] -> [email protected] -> [email protected], released and you want to update B and C to latest within their semver ranges.
How do you do this?

yarn upgrade A would only bump A to 1.2.1 and then grab [email protected] and [email protected] from yarn.lock if [email protected] did not update its dependency on B.

Removing B and C sections from the lockfile manually seems like the easiest solution to bump them.

Is it worth having a command for this?
Some ideas:

  • yarn unlock A@^1.2.0 - would remove A and all subdependencies from yarn.lock and on next install Yarn would fetch latest
  • yarn upgrade A --fresh/transitive - would update A and re-resolve all subdependencies ignoring all existing entries in yarn.lock. That might be tricky because the resolution logic is parallel and recursive

@dfreeman
Copy link

dfreeman commented Jul 5, 2017

As long as yarn.lock continues to open with DO NOT EDIT THIS FILE DIRECTLY (which I assume isn't changing, nor would I want it to), it seems like having a command to manage this would be beneficial.

I think my ideal would be something that captures the behavior you describe for yarn unlock and then immediately does an install — is there a use case I'm not imagining where you'd only want to do the unlock and then stay in a state where you have missing yarn.lock entries?

@arcanis
Copy link
Member

arcanis commented Jul 17, 2017

yarn upgrade A --fresh/transitive - would update A and re-resolve all subdependencies ignoring all existing entries in yarn.lock. That might be tricky because the resolution logic is parallel and recursive

I'm not sure this would be possible, because the lockfile isn't a tree. Let's say two dependencies (A and B) both depend on C@^1.0.0, and are both locked to 1.0.0. Running upgrade A --deep would then proceed to upgrade both A/node_modules/C and B/node_modules/C to 1.0.1, even though we requested to only upgrade the first one 😞

@jesstelford
Copy link

jesstelford commented Aug 30, 2017

@arcanis I think that would be an acceptable trade-off. I imagine this command would only be run very rarely, and only when the dev really knows they want to update the deps - it's an advanced level command. So it's ok to make tradeoffs like that.

It'd probably be a good idea to get confirmation first:

$ yarn unlock A@^1.0.2
This will unlock A for the following dependant packages:
└─┬ [email protected]
  └── [email protected]
└─┬ [email protected]
  └─┬ [email protected]
    └── [email protected] 
Are you sure? [y]

@BYK BYK self-assigned this Sep 29, 2017
@marijnh
Copy link

marijnh commented Feb 23, 2018

I am not sure just having a command-line flag here would be enough. I've been getting a lot of bug reports on one of my libraries where a dependency of the library was, at some point, broken. A fixed version has been released a long time ago, but people are still stuck on the old version because yarn will keep the old version of the transitive dependency in the package lock around... forever, it seems, unless you do something non-obvious like removing the library and re-installing it.

I think even when we have the proposed command-line option (if this rfc picks up stream again at some point) the fact that users have to find out about it and use it to upgrade is a liability. Is there a good reason not to do this by default when running yarn upgrade?


# Summary

This RFC proposes the addition of a "update-transitive" command to the Yarn CLI.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be upgrade-transitive to match upgrade and upgrade-interactive, otherwise it looks like it goes with yarn self-update

Copy link
Author

@magicmark magicmark Mar 19, 2018

Choose a reason for hiding this comment

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

Yep, this makes sense, thanks! I've updated this in the proposal.

@magicmark magicmark changed the title Add '--transitive' to yarn upgrade-interactive Add proposal for new CLI command: yarn upgrade-transitive Mar 19, 2018
@magicmark
Copy link
Author

So quite a lot has changed since the initial draft of this RFC - namely the introduction of the new Selective dependency resolutions API.

I think it makes sense to use this existing API to solve the problems of force upgrading a transitive dep past its usual semver range.

A new CLI command/flag wrapping this might still be a nice DUX thing. Ideally it would also bump an in-range version of a transitive dep without adding an override. This would unify the workflow for both scenarios under one command.

I'll let this marinate for a few days to get some feedback on this, and I'll do some investigation into some implementation details here.

roschaefer added a commit to roschaefer/human-connection-api-nodejs-client that referenced this pull request Apr 29, 2018
Vulnerability: https://nvd.nist.gov/vuln/detail/CVE-2018-3728

`rm yarn.lock`
`yarn install`

There is currently no way to update single transitive dependencies
see: yarnpkg/rfcs#54
roschaefer added a commit to roschaefer/API that referenced this pull request Apr 29, 2018
Vulnerability: https://nvd.nist.gov/vuln/detail/CVE-2018-3728

`rm yarn.lock`
`yarn install`

There is currently no way to update single transitive dependencies
see: yarnpkg/rfcs#54
@magicmark
Copy link
Author

magicmark commented May 3, 2018

@bestander Updated the RFC and trimmed it down a bit. I think it's in a good place to be reviewed again.

(tl;dr this is mostly now a dumb wrapper around the package.resolutions field, with some optimizations planned as a follow-up.)

Thanks everyone who has looked over this so far! (And thanks to @kaylieEB @victornoel for the work on the resolutions field that this will use 😃 )

@mydea
Copy link

mydea commented Oct 9, 2018

I don't quite see how this is the same as the resolutions in the package.json. We find ourselves in this scenario quite often and it is frankly rather annoying:

We have multiple dependencies with the same dependency, for example:

  • my-app:
    • shared-dep@~2.0.0
    • depA:
      • shared-dep@~2.0.0
    • depB:
      • shared-dep@~2.0.0
    • depC:
      • shared-dep@^2.0.3

Now we sometimes end up with two versions of shared-dep (e.g. 2.0.0 and 2.0.3) in our yarn.lock file - and there is no way to get rid of this, as running e.g. yarn upgrade shared-dep will ignore the transitive dependencies. The only way we found to really get rid of this is to run yarn upgrade, but this will upgrade all dependencies - we'd like to sometimes only upgrade certain dependencies.

This feels really weird, since all of these can easily be satisfied by the same version (e.g. 2.0.4), and it does not feel clear to me at all why running yarn upgrade shared-dep would not also try to upgrade transitive dependencies.

IMHO, requiring to manually delete entries from the yarn.lock file is not really a solution for that. Also, I don't see how requiring us to add a manual resolution to the package.json is an adequate solution for that - since we'll need to then update this resolution for each further update of the dependency, which adds a lot of noise.

We would love to have any one of these options:

  • something like yarn unlock to remove an entry from the lockfile
  • Ensure that e.g. yarn upgrade depC would also upgrade this package's dependencies to the allowed ranges (or add a command option to activate that behavior)
  • Add a new yarn upgrade-transitive command to accomodate this

@dfreeman
Copy link

dfreeman commented Oct 9, 2018

Maybe relevant: yarn dedupe is already a command, I'm guessing because of the existence of the npm equivalent. Today when you run yarn dedupe it no-ops and emits a message claiming to be unnecessary, but the problem described here seems like more or less what npm dedupe is meant to solve.

@yched
Copy link

yched commented Oct 10, 2018

@mydea I'm not sure what you describe is really related to the issue here, but I'm seeing it as well and it is indeed fairly annoying. I've been using yarn-tools (through npx) to fix this "semi-manually"

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.