Upgrade version rework 3603 #3847
Conversation
| commander.usage('upgrade-interactive'); | ||
| commander.option('-E, --exact', 'upgrade to most recent release with exact version'); | ||
| commander.option('-T, --tilde', 'upgrade to most recent release with patch version'); | ||
| setUpgradeFlags(commander); |
There was a problem hiding this comment.
I'm not fond - I see why you're doing this, but it makes it harder to know what are the command options when reading the source code
There was a problem hiding this comment.
Yeah I was just trying to avoid the duplication. I can put the flags in both commands though.
src/cli/commands/upgrade.js
Outdated
| const deps = await getOutdated(config, reporter, flags, lockfile, args); | ||
|
|
||
| // if specific versions were requested for packages, override what getOutdated reported as the latest to install | ||
| args.forEach(requestedPattern => { |
There was a problem hiding this comment.
We should try to use for (let ... of ...) instead of forEach when possible
There was a problem hiding this comment.
Is there a reason, or just personal preference? I always visually mistake for ... of ... as the old for ... in ... which you seldom actually want. The subtle difference always messes me up.
src/cli/commands/upgrade.js
Outdated
| current: '', | ||
| latestPattern: newPattern, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Do we want to do this? I'd expect yarn upgrade foo@1.0.0 to throw if foo doesn't exists
There was a problem hiding this comment.
This was the case where a user specifies an exact version for something that isn't outdated. For example if you have foo@2.0.0 and that happens to be the latest, it won't be returned by getOutdatedPackages, but the user can still do yarn upgrade foo@1.2.4 and we would change to that requested version.
This is preserving functionality that the previous upgrade had (because it would just forward the package with version through to add)
src/cli/commands/upgrade.js
Outdated
| // instead of the latest for the range. | ||
| deps.forEach(dep => lockfile.removePattern(dep.latestPattern)); | ||
|
|
||
| const addFlags = Object.assign({}, flags, {force: true}); |
There was a problem hiding this comment.
Can harmful flags be passed to upgrade by mistake?
There was a problem hiding this comment.
Maybe. Probably. This is what the old/existing code did so I preserved it without much thought. Does the commander library that handles the command line options have anything to enforce only the allowed flags?
src/cli/commands/upgrade.js
Outdated
|
|
||
| const addFlags = Object.assign({}, flags, {force: true}); | ||
| const addArgs = deps.map(dep => dep.latestPattern); | ||
| delete addFlags.latest; |
There was a problem hiding this comment.
You can just use Object.assign({}, flags, {force: true, latest: false}) above to avoid using delete
src/types.js
Outdated
| @@ -153,6 +153,8 @@ export type Dependency = { | |||
| latest: string, | |||
| url: string, | |||
| hint: ?string, | |||
| range: string, | |||
| latestPattern: string, | |||
There was a problem hiding this comment.
(+ in the rest of the code)
I feel like the name isn't very clear - the difference between latest and latestPattern should be explained somewhere
There was a problem hiding this comment.
yeah, I agree... I had a hard time coming up with a meaningful name. Any suggestions? It's basically the name@version that we will upgrade to, which gets passed as input to the add command. Maybe something like upgradeToPattern or just upgradeTo?
|
Don't forget that automated tests (at least on |
|
@rally25rs Let me know when you want another review! Would love to see this merged 👍 |
|
@arcanis I did some refactoring and linked a PR for the docs. |
| const deps = (await PackageRequest.getOutdatedPackages(lockfile, install, config, reporter, patterns)) | ||
| .filter(versionFilter) | ||
| .filter(scopeFilter); | ||
| deps.forEach(dep => (dep.upgradeTo = buildPatternToUpgradeTo(dep, flags))); |
There was a problem hiding this comment.
It won't block the merge but I think we should try using for ... of when possible, instead of forEach
|
Thanks a lot, @rally25rs ! I'm really happy to see this merged ! |
|
Great work @rally25rs, thank you so much for your contributions to Yarn :) |
|
This has changed what The only way to achieve the old behaviour is to remove |
** Related PR **
Documentation changes: yarnpkg/website#578
Summary
This is a PR for work done so far for RFC: https://github.com/yarnpkg/rfcs/blob/master/accepted/0000-upgrade-command-consistency.md
upgradecommand now works more likeupgrade-interactive.upgrade-interactiveis now actually an interactive version ofupgrade.See RFC for details.