Fix 4263: Make upgrade and upgrade-interactive compatible with workspaces#4278
Conversation
| pushDeps('optionalDependencies', projectManifestJson, {hint: 'optional', optional: true}, true); | ||
|
|
||
| if (this.config.workspaceRootFolder) { | ||
| if (this.config.workspaceRootFolder && !this.ignoreWorkspaces) { |
There was a problem hiding this comment.
Hm, doesn't this break running yarn install at the root of the workspace? It won't install the workspaces dependencies anymore, and its own dependencies won't have the same layout than what they would have been if the hoister had been aware of the workspaces.
There was a problem hiding this comment.
this.ignoreWorkspaces is only set on add (defaults to false), so install should be ok. I'd also imagine this test (https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/install/workspaces-install.js#L71) would catch it?
There was a problem hiding this comment.
Oh I see! Ok, I missed that ignoreWorkspaces was undefined in Install. I think you should make it a flag defined in Install (rather than Add) and add a setter to toggle it (like the topLevelBinLinkFlag implemented here).
6448d3c to
3bd3602
Compare
| if (deps.length) { | ||
| for (const pattern of deps) { | ||
| lockfile.removePattern(pattern); | ||
| } |
There was a problem hiding this comment.
upgrade-interactive seemed to have been previously broken, regardless of workspaces or not. When --latest flag wasn't being passed, it needed https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/upgrade.js#L84-L86, like upgrade, otherwise nothing would get re-installed.
|
|
||
| if (flags.existing) { | ||
| this.flagToOrigin = ''; | ||
| } |
There was a problem hiding this comment.
So the issue I was running into was that when I ran yarn upgrade typescript (a dev dependency) in the workspace root directory, it would incorrectly warn me that workspaces prefer dev dependencies. This is because flagsToOrgin defaults to being a hard dependency when flags aren't passed. I don't think yarn should ever throw that warning if the command is an upgrade and save it for when it's a brand new add (hence the check for flags.existing passed in the upgrade file below).
There was a problem hiding this comment.
An inline comment about this would have been great.
|
|
||
| async init(): Promise<Array<string>> { | ||
| if (this.config.workspaceRootFolder && this.config.cwd === this.config.workspaceRootFolder) { | ||
| if (this.ignoreWorkspaces) { |
There was a problem hiding this comment.
Unless I'm mistaken, this condition cannot be replaced this way; it is meant to ensure that running yarn add <something> inside the workspace root will warn the user that they probably don't want to do this. With this change, no warning will be emitted in such a case because ignoreWorkspaces will be false in traditional add settings. I think a better condition is:
this.config.workspaceRootFolder && this.config.cwd === this.config.workspaceRootFolder && !this.ignoreWorkspacesThere was a problem hiding this comment.
That conditional wouldn't ever be true though according to this logic right? https://github.com/kaylieEB/yarn/blob/93b2aaeb33aad23218177f3a1ed2bd9870e4d077/src/cli/commands/add.js#L27-L29. I've also tested that it still warns when I try to add a non-dev dependency.
arcanis
left a comment
There was a problem hiding this comment.
I have some concerns on the readability, but let's merge it for now and we can iterate later :)
|
How would one run |
…aces (yarnpkg#4278) * make upgrade and upgrade-interactive compatible with workspaces * lint * flow * feedback from @arcanis * fix flow * remove lockfile pattern for normal upgrade interactive
…g#4318) * add comments * rewording
Summary
Fix for #4263
When called at the project root level with workspaces, upgrade and upgrade-interactive should only change the root dev dependencies. Previously it was resolving / reinstalling workspace dependencies as well. Also it didn't allow for existing dev dependency to update (see screenshot)
Test plan
Added a test in
upgradewith workspacesBEFORE (only chalk should be updated)
AFTER
BEFORE (typescript is a dev dependency)
AFTER