Support `--peer` for install to install peer deps as well #4689

pvdz · 2017-10-11T18:20:06Z

Summary

This fixes #1503
This also drops some legacy options for help output (which were moved to add and don't work at all under install)

Test plan

Have a package with peerDepdendencies, install with yarn install --peer and confirm that the packages listed under peerDepdendencies are also installed.
Test cases added.

This fixes yarnpkg#1503 This also drops some legacy options for help output (which were moved to `add`) This refactors `run` a little because the code was a bit spaghetti pasta Added a test

pvdz · 2017-10-11T18:21:26Z

Wasn't sure how to add a test for mock packages. Would like some input on that. I'd prefer not to use actual packages as I only want to see that the peer packages were or weren't included.

arcanis · 2017-10-12T10:09:44Z

I think I like it, but what do you think about making this a default behavior rather than something hidden behind a flag? Since it only affects the top-level package (which shouldn't have peer dependencies anyway, unless it's a plugin of something in which case it would make sense to install the peer dependencies), that shouldn't change a lot of things.

The only things I can see being affected would be people using yarn link (because then the linked plugin would use the official release instead of the one that linked it), but then they would just have to run yarn link in their plugin as well to setup the cross-link.

Would probably require a major bump, but I think it's worth being considered.

arcanis · 2017-10-12T10:10:33Z

As for the tests, you can setup a fixture that contains sub-packages and reference them through the file: protocol.

BYK

I'm concerned about this causing a breaking change.

Also, we need to separate the actual fix with code cleanups and improvements in other places, especially ones removing existing functionality 😉

BYK · 2017-10-12T14:59:52Z

src/cli/commands/install.js

  commander.option('-g, --global', 'DEPRECATED');
-  commander.option('-S, --save', 'DEPRECATED - save package to your `dependencies`');


Why remove these? They are deprecated but removing them is a breaking change and we don't want to release a major version just yet.

They aren't deprecated so much as simply invalid. They won't do anything anymore so why report them as deprecated?

The --peer option was going to be re-used in light of this and the other options are only valid in add, unless I'm mistaken?

I left -g because I think that still makes sense with install. Although I think that one is also moot?

Removing deprecated features doesn't require a major per semver. I don't have a problem removing them, since it's been pretty clear for quite a long time now that yarn install isn't meant to add dependencies.

Wait, I thought these options did not work at all with install. That's why I removed their listing. If they still function then I stand corrected and they should remain until the feature itself is removed.

(Note that this only removes the --help output, not the actual features they once represented...)

Removing deprecated features doesn't require a major per semver. I don't have a problem removing them, since it's been pretty clear for quite a long time now that yarn install isn't meant to add dependencies.

I think this is still a hard-breaking change. I'll ask React guys to see what they do. I think they start deprecating in feature updates and only remove them in major releases which makes sense.

Wait, I thought these options did not work at all with install. That's why I removed their listing. If they still function then I stand corrected and they should remain until the feature itself is removed.

The feature is always there. These are just legacy flags from NPM but AFAIK they do work.

They don't work per-se, but generate a 'suggested command line'.

https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L951-L975

@qfox Can you remove their removal from this PR? I think we should focus on the peer dependency itself, removing the deprecated commands will be a separate PR :)

BYK · 2017-10-12T15:00:20Z

src/cli/commands/install.js

  if (args.length) {
-    const exampleArgs = args.slice();
+    let command = 'add';
+    let error = 'installCommandRenamed';


Is this change relevant to the new feature?

No. Guess I'll remove the refactor.

BYK · 2017-10-12T15:02:53Z

src/cli/commands/install.js

@@ -344,6 +351,9 @@ export class Install {
            pushDeps('dependencies', workspaceManifest, {hint: null, optional: false}, true);
            pushDeps('devDependencies', workspaceManifest, {hint: 'dev', optional: false}, !this.config.production);
            pushDeps('optionalDependencies', workspaceManifest, {hint: 'optional', optional: true}, true);
+            if (this.flags.includePeerDeps) {


@arcanis I feel like this would change the lockfile resolution based on the flag which is not something we want.

I feel like we should drop the if block and use the following line:

pushDeps('peerDependencies', workspaceManifest, {hint: 'peer', optional: false}, this.flags.includePeerDeps);

Even then, I feel like this will be a big change in the final resolution. What do you think?

That's a good point. Whether it's a valid point is probably going to be a plain decision by the product owner.

Either we support this and running the command will invalidate the cache if used inconsistently (with and without the flag). The user may complain about the cache being broken, or it may not notice it at all. We could print a warning for this if that's desirable.

Or we don't support this feature. In that case we should mention a wontfix in the ticket, perhaps state the reason why, and close it.

I don't think it's worth a major version bump for making this a default behavior (see #4689 (comment))

@BYK I think there is a precedent for this (namely, the --flat option). But that's something that would be solved if peer dependencies were installed by default rather than via a flag (which I still think would make sense).

pvdz · 2017-10-12T15:19:50Z

@arcanis

but what do you think about making this a default behavior

As you say yourself, the whole thing doesn't happen too often. The flag pleases a small subset of people who explicitly want to install the peer deps as well. As I understand it, this is normally not the case for most people. Making it a default behavior could actually be detrimental since you may be installing something local which is already available globally, and now you have two versions to keep to update.

If making this a default behavior may also affect other cases like link then that seems like all the more reasons not to make this a default behavior.

Additionally, if the use case is so minor then a major version bump seems definitely unwarranted.

I think the flag is fine.

I'll have another look at the test.

pvdz · 2017-10-13T18:48:48Z

Updated with proper tests and sans the refactoring.

Still dropping the flags since I think they can't be used without add. So it's not a breaking change to remove the help messages, merely cleanup.

arcanis · 2017-10-30T12:30:09Z

src/cli/commands/install.js

@@ -344,6 +351,9 @@ export class Install {
            pushDeps('dependencies', workspaceManifest, {hint: null, optional: false}, true);
            pushDeps('devDependencies', workspaceManifest, {hint: 'dev', optional: false}, !this.config.production);
            pushDeps('optionalDependencies', workspaceManifest, {hint: 'optional', optional: true}, true);
+            if (this.flags.includePeerDeps) {


@BYK I think there is a precedent for this (namely, the --flat option). But that's something that would be solved if peer dependencies were installed by default rather than via a flag (which I still think would make sense).

arcanis · 2017-10-30T12:31:39Z

src/cli/commands/install.js

  commander.option('-g, --global', 'DEPRECATED');
-  commander.option('-S, --save', 'DEPRECATED - save package to your `dependencies`');


They don't work per-se, but generate a 'suggested command line'.

https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L951-L975

@qfox Can you remove their removal from this PR? I think we should focus on the peer dependency itself, removing the deprecated commands will be a separate PR :)

arcanis · 2018-02-27T17:53:28Z

cc @qfox ? do you think you'll have the bandwidth to rebase this PR and make the few changes requested ?

pvdz · 2018-04-03T07:52:32Z

@arcanis I don't think I have the necessary bw to push this further. I remember from discussions in our team that this is a more complex and contentious feature to work on than I have time to flesh out.

If somebody else feels this is important and wants to finish where I left of then please feel free to do so. You can use my code, I'll keep the branch up.

I am closing this PR since I won't see myself working on it any further any time soon. If this feature is important to you please take my code, rebase it, apply the changes as suggested above, and polish it. Thanks :)

Support --peer for install to install peer deps as well

93ced1d

This fixes yarnpkg#1503 This also drops some legacy options for help output (which were moved to `add`) This refactors `run` a little because the code was a bit spaghetti pasta Added a test

BYK requested changes Oct 12, 2017

View reviewed changes

BYK requested a review from arcanis October 12, 2017 15:04

BYK assigned BYK and arcanis Oct 12, 2017

Peter van der Zee added 2 commits October 12, 2017 15:51

Undo refactor

51b3781

Add proper tests with mock packages

ce1fa98

eriktrom mentioned this pull request Oct 18, 2017

Peer and Optional Dependencies Support ember-cli/rfcs#112

Closed

arcanis requested changes Oct 30, 2017

View reviewed changes

pvdz closed this Apr 3, 2018

LauraBeatris mentioned this pull request Aug 8, 2020

Peer-dependencies not being installed jungsoft/eslint-config#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `--peer` for install to install peer deps as well #4689

Support `--peer` for install to install peer deps as well #4689

pvdz commented Oct 11, 2017 •

edited

Loading

pvdz commented Oct 11, 2017

arcanis commented Oct 12, 2017

arcanis commented Oct 12, 2017

BYK left a comment

BYK Oct 12, 2017

pvdz Oct 12, 2017

arcanis Oct 12, 2017

pvdz Oct 12, 2017

BYK Oct 26, 2017

arcanis Oct 30, 2017

BYK Oct 12, 2017

pvdz Oct 12, 2017

BYK Oct 12, 2017

pvdz Oct 12, 2017

arcanis Oct 30, 2017

pvdz commented Oct 12, 2017

pvdz commented Oct 13, 2017

arcanis Oct 30, 2017

arcanis Oct 30, 2017

arcanis commented Feb 27, 2018

pvdz commented Apr 3, 2018

		commander.option('-g, --global', 'DEPRECATED');
		commander.option('-S, --save', 'DEPRECATED - save package to your `dependencies`');

Support --peer for install to install peer deps as well #4689

Support --peer for install to install peer deps as well #4689

Conversation

pvdz commented Oct 11, 2017 • edited Loading

pvdz commented Oct 11, 2017

arcanis commented Oct 12, 2017

arcanis commented Oct 12, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdz commented Oct 12, 2017

pvdz commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented Feb 27, 2018

pvdz commented Apr 3, 2018

Support `--peer` for install to install peer deps as well #4689

Support `--peer` for install to install peer deps as well #4689

pvdz commented Oct 11, 2017 •

edited

Loading