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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93ced1d
Support `--peer` for install to install peer deps as well
Oct 11, 2017
51b3781
Undo refactor
Oct 12, 2017
ce1fa98
Add proper tests with mock packages
Oct 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions __tests__/commands/install/integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,3 +1100,17 @@ test.concurrent('warns for missing bundledDependencies', (): Promise<void> => {
'missing-bundled-dep',
);
});

test.concurrent('install should not install peer deps by default', async () => {
await runInstall({}, 'peer-deps-opt', async (config): Promise<void> => {
expect(await fs.exists(`${config.cwd}/node_modules/commander`)).toEqual(true);
expect(await fs.exists(`${config.cwd}/node_modules/qs`)).toEqual(false);
});
});

test.concurrent('install should install peer deps when flag is present', async () => {
await runInstall({peer: true}, 'peer-deps-opt', async (config): Promise<void> => {
expect(await fs.exists(`${config.cwd}/node_modules/commander`)).toEqual(true);
expect(await fs.exists(`${config.cwd}/node_modules/qs`)).toEqual(true);
});
});
11 changes: 11 additions & 0 deletions __tests__/fixtures/install/peer-deps-opt/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "peer-opt",
"version": "1.0.0",
"license": "MIT",
"dependencies": {
"commander": "^2.9.0"
},
"peerDependencies": {
"qs": "6.3.0"
}
}
38 changes: 22 additions & 16 deletions src/cli/commands/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ function normalizeFlags(config: Config, rawFlags: Object): Flags {
frozenLockfile: !!rawFlags.frozenLockfile,
linkDuplicates: !!rawFlags.linkDuplicates,
checkFiles: !!rawFlags.checkFiles,
includePeerDeps: !!rawFlags.peer,

// add
peer: !!rawFlags.peer,
Expand Down Expand Up @@ -254,6 +255,9 @@ export class Install {
if (manifest.optionalDependencies && manifest.optionalDependencies[exclude]) {
delete manifest.optionalDependencies[exclude];
}
if (manifest.peerDependencies && manifest.peerDependencies[exclude]) {
delete manifest.peerDependencies[exclude];
}
}
};

Expand Down Expand Up @@ -318,6 +322,9 @@ export class Install {
pushDeps('dependencies', projectManifestJson, {hint: null, optional: false}, true);
pushDeps('devDependencies', projectManifestJson, {hint: 'dev', optional: false}, !this.config.production);
pushDeps('optionalDependencies', projectManifestJson, {hint: 'optional', optional: true}, true);
if (this.flags.includePeerDeps) {
pushDeps('peerDependencies', projectManifestJson, {hint: 'peer', optional: false}, !this.config.production);
}

if (this.config.workspaceRootFolder) {
const workspaceLoc = cwdIsRoot ? loc : path.join(this.config.lockfileFolder, filename);
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Author

Choose a reason for hiding this comment

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

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))

Copy link
Member

Choose a reason for hiding this comment

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

@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).

pushDeps('peerDependencies', workspaceManifest, {hint: 'peer', optional: false}, !this.config.production);
}
}
}
const virtualDependencyManifest: Manifest = {
Expand All @@ -355,6 +365,7 @@ export class Install {
dependencies: workspaceDependencies,
devDependencies: {...workspaceManifestJson.devDependencies},
optionalDependencies: {...workspaceManifestJson.optionalDependencies},
peerDependencies: this.flags.includePeerDeps ? {...workspaceManifestJson.peerDependencies} : {},
};
workspaceLayout.virtualManifestName = virtualDependencyManifest.name;
const virtualDep = {};
Expand Down Expand Up @@ -917,13 +928,8 @@ export function hasWrapper(commander: Object, args: Array<string>): boolean {

export function setFlags(commander: Object) {
commander.usage('install [flags]');
commander.option('--peer', 'explicitly install peer dependencies as well');
commander.option('-g, --global', 'DEPRECATED');
commander.option('-S, --save', 'DEPRECATED - save package to your `dependencies`');
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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...)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

commander.option('-D, --save-dev', 'DEPRECATED - save package to your `devDependencies`');
commander.option('-P, --save-peer', 'DEPRECATED - save package to your `peerDependencies`');
commander.option('-O, --save-optional', 'DEPRECATED - save package to your `optionalDependencies`');
commander.option('-E, --save-exact', 'DEPRECATED');
commander.option('-T, --save-tilde', 'DEPRECATED');
}

export async function install(config: Config, reporter: Reporter, flags: Object, lockfile: Lockfile): Promise<void> {
Expand All @@ -934,16 +940,10 @@ export async function install(config: Config, reporter: Reporter, flags: Object,
}

export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
let lockfile;
let error = 'installCommandRenamed';
if (flags.lockfile === false) {
lockfile = new Lockfile();
} else {
lockfile = await Lockfile.fromDirectory(config.lockfileFolder, reporter);
}

if (args.length) {
const exampleArgs = args.slice();
let command = 'add';
let error = 'installCommandRenamed';
Copy link
Member

Choose a reason for hiding this comment

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

Is this change relevant to the new feature?

Copy link
Author

Choose a reason for hiding this comment

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

No. Guess I'll remove the refactor.

let exampleArgs = args.slice();

if (flags.saveDev) {
exampleArgs.push('--dev');
Expand All @@ -960,14 +960,20 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
if (flags.saveTilde) {
exampleArgs.push('--tilde');
}
let command = 'add';

if (flags.global) {
error = 'globalFlagRemoved';
command = 'global add';
}
throw new MessageError(reporter.lang(error, `yarn ${command} ${exampleArgs.join(' ')}`));
}

let lockfile;
if (flags.lockfile === false) {
lockfile = new Lockfile();
} else {
lockfile = await Lockfile.fromDirectory(config.lockfileFolder, reporter);
}
await install(config, reporter, flags, lockfile);
}

Expand Down