feat(workspaces): Add `--ignore-workspace-root-check` flag and improve usage by arcanis · Pull Request #4630 · yarnpkg/yarn

arcanis · 2017-10-04T13:20:15Z

Summary

Supercedes #4583 - cc @kaylieEB and @jgoz

Implements a --ignore-workspace-root-check option that replaces the --dev requirement when adding a dependency at the root. We received a lot of feedback about this warning, which prompted this change. You can now add both production and development dependencies to your packages, as long as you also use the new flag. Hopefully it will make things easier to understand!
Fixes the bug of Fix commands when used in a workspace #4583 where the extraneous files were not removed from the workspaces.
Partially fixes outdated, which will now print the whole tree (including the workspace root).
Running yarn add in the root won't remove workspaces dependencies from the lockfile anymore.

Details

At first this PR was only about adding the --ignore-workspace-root-check flag, but then I needed to change some parts of the code that #4583 was already altering, so I had to spend some time thinking about it as well, and eventually integrated the relevant changes here.

First, a distinction has to be made:

First we have the package.json file format. We have a package.json for each workspace.
Then we have the yarn.lock file format. There's only a single lockfile that aggregate all workspaces.

Which now brings me to my point: the following bugs, fixed in #4278 and #4583:

"When called at the project root level with workspaces, upgrade and upgrade-interactive should only change the root dev dependencies"
"upgrade to workspace root preserves child dependencies"
"upgrade to workspace child preserves root dependencies"

They are not bugs.

Now that might sounds weird, but it all makes sense if you think about how we store the resolved dependencies! Our lockfile format looks like this:

"foo@~1.0.0"
    version: 1.0.0

Think about a workspace root containing two workspaces, A and B. Both depends on foo@~1.0.0. Now what will happen when we run yarn upgrade foo in A? Should we only update A? But then, what will our lockfile look like? We can't have two entries with the same key, like this:

"foo@~1.0.0"
    version: 1.0.0

"foo@~1.0.0"
    version: 1.0.5

And here lies the problem: the lockfile format doesn't (yet) allow us to operate on part of the workspace tree. We always must deal with the whole workspace tree, because if we don't we'll end up corrupting the lockfile by "forgetting" entries (like what happened when we were just not reading the workspaces package.json files), or overriding them accidentally (like what would happen if we were to just override foo@~1.0.0: B would be surprisingly affected as well by this change because it happens to share the same pattern range).

Test plan

Added some tests from #4583, fixed a few others, everything should be green.

buildsize · 2017-10-04T13:55:59Z

This change will decrease the build size from 9.91 MB to 9.9 MB, a decrease of 2.96 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	856.9 KB	856.41 KB	-497 bytes (0%)
yarn-[version].js	3.77 MB	3.77 MB	-905 bytes (0%)
yarn-legacy-[version].js	3.82 MB	3.82 MB	-879 bytes (0%)
yarn-v[version].tar.gz	862.77 KB	862.33 KB	-455 bytes (0%)
yarn_[version]all.deb	652.13 KB	651.85 KB	-292 bytes (0%)

buildsize · 2017-10-04T13:55:59Z

This change will decrease the build size from 9.91 MB to 9.9 MB, a decrease of 2.96 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	856.9 KB	856.41 KB	-497 bytes (0%)
yarn-[version].js	3.77 MB	3.77 MB	-905 bytes (0%)
yarn-legacy-[version].js	3.82 MB	3.82 MB	-879 bytes (0%)
yarn-v[version].tar.gz	862.77 KB	862.33 KB	-455 bytes (0%)
yarn_[version]all.deb	652.13 KB	651.85 KB	-292 bytes (0%)

BYK

LGTM. Great job with the PR explanation.

BYK · 2017-10-04T14:00:09Z

src/cli/commands/add.js


 export function setFlags(commander: Object) {
  commander.usage('add [packages ...] [flags]');
+  commander.option('-W, --ignore-workspace-root-check', 'required to run yarn add inside a workspace root');


Interesting short-hand flag name.

Didn't think long about this one, but you make me remember it conflicts with -W(arning) of compilers 😞 oh well, should be fine for this iteration.

BYK · 2017-10-04T14:03:17Z

src/package-linker.js

-          if (file[0] === '@') {
+    const scopedPaths: Set<string> = new Set();
+
+    const findExtraneousFiles = async basePath => {


It may be better in the future to extract this method outside of the closure.

Since it needs to changes scopedPaths and extraneousFiles and isn't meant to be reused anywhere else in the code I thought it was cleaner to keep it together with the code segment that uses it.

jgoz

@arcanis Your explanation makes sense and I agree that outdated and upgrade should operate across all workspaces, including the root.

However, I just tested your branch and ran outdated and upgrade-interactive at the workspace root for my test repository, and both commands only showed outdated root packages only, even though there are outdated packages in the workspaces. Did you intend to change that behaviour in this PR too?

jgoz

Also, this doesn't fix the "No lockfile in this directory..." issue when running remove inside of a workspace package (which is a legitimate case for running an individual command in a workspace).

I think you need to include the changes I made in cli.js too.

arcanis · 2017-10-05T14:58:35Z

@jgoz Yep I missed those fixes :/ Right now I'll have to merge this PR as is (the new yarn add option is important for common use cases), would you mind opening a new PR with the things I missed? I'm sorry, I know it must be time-consuming 😞

jgoz · 2017-10-05T15:14:28Z

@arcanis No problem! I can open a new PR that will look very similar to #4583 😉

edmorley · 2017-10-05T15:25:45Z

Thank you both for all your work on this - I can't wait to start using yarn workspaces once the remaining papercuts are fixed 😃

Looking at #4583 which this superseded, I see a few test-cases that weren't migrated to this PR - was this intentional? eg:

for outdated: https://github.com/yarnpkg/yarn/pull/4583/files#diff-b803c2733e818112a7578aac3382f13cR134
for remove: https://github.com/yarnpkg/yarn/pull/4583/files#diff-314ee529e34b31b97ee889bb502f5b8fR319

jgoz · 2017-10-05T16:03:07Z

@edmorley I plan to migrate those fixes to a new PR shortly, though outdated will work as @arcanis suggested in the description of this PR (i.e., it will operate across the whole workspace, both root and individual packages).

arcanis requested review from BYK and kaylie-alexa October 4, 2017 13:20

arcanis assigned arcanis and unassigned arcanis Oct 4, 2017

Improves the workspaces

8ba4966

arcanis force-pushed the yarn-workspace-improvs branch from 7647e38 to 8ba4966 Compare October 4, 2017 13:44

BYK approved these changes Oct 4, 2017

View reviewed changes

BYK changed the title ~~Improves the workspaces~~ feat(workspaces): Add --ignore-workspace-root-check flag and improve the DX Oct 4, 2017

BYK changed the title ~~feat(workspaces): Add --ignore-workspace-root-check flag and improve the DX~~ feat(workspaces): Add --ignore-workspace-root-check flag and improve usage Oct 4, 2017

jgoz reviewed Oct 4, 2017

View reviewed changes

jgoz suggested changes Oct 4, 2017

View reviewed changes

arcanis mentioned this pull request Oct 5, 2017

Cannot add a regular dependency to a workspace root #4558

Closed

arcanis merged commit e286034 into yarnpkg:master Oct 5, 2017

arcanis mentioned this pull request Oct 5, 2017

Fix commands when used in a workspace #4583

Closed

jgoz mentioned this pull request Oct 6, 2017

Fix: workspace support in several commands #4654

Merged

edmorley mentioned this pull request Oct 17, 2017

Root binaries get removed after installing a Workspace dependency #4706

Closed

joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017

Improves the workspaces (yarnpkg#4630)

a9e3675

verkholantsev mentioned this pull request Mar 27, 2018

Proposal: Add ignore-workspace-root-check option to .yarnrc #5578

Closed

Conversation

arcanis commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

buildsize bot commented Oct 4, 2017

Uh oh!

buildsize bot commented Oct 4, 2017

Uh oh!

BYK left a comment

Choose a reason for hiding this comment

Uh oh!

BYK Oct 4, 2017

Choose a reason for hiding this comment

Uh oh!

arcanis Oct 4, 2017

Choose a reason for hiding this comment

Uh oh!

BYK Oct 4, 2017

Choose a reason for hiding this comment

Uh oh!

arcanis Oct 4, 2017

Choose a reason for hiding this comment

Uh oh!

jgoz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgoz left a comment

Choose a reason for hiding this comment

Uh oh!

arcanis commented Oct 5, 2017

Uh oh!

jgoz commented Oct 5, 2017

Uh oh!

edmorley commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgoz commented Oct 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

arcanis commented Oct 4, 2017 •

edited

Loading

jgoz left a comment •

edited

Loading

edmorley commented Oct 5, 2017 •

edited

Loading