Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
16 changes: 16 additions & 0 deletions __tests__/commands/install/integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,14 @@ test.concurrent('install a module with incompatible optional dependency should s
});
});

test.concurrent('install a module with optional dependency should skip incompatible transient dependency',
(): Promise<void> => {
return runInstall({}, 'install-should-skip-incompatible-optional-sub-dep', async (config) => {
assert.ok(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-optional')));
assert.ok(!(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-incompatible'))));
});
});

// this tests for a problem occuring due to optional dependency incompatible with os, in this case fsevents
// this would fail on os's incompatible with fsevents, which is everything except osx.
if (process.platform !== 'darwin') {
Expand All @@ -843,6 +851,14 @@ test.concurrent('optional dependency that fails to build should not be installed
});
});

test.concurrent('failing dependency of optional dependency should not be installed',
(): Promise<void> => {
return runInstall({}, 'should-not-install-failing-deps-of-optional-deps', async (config) => {
assert.equal(await fs.exists(path.join(config.cwd, 'node_modules', 'optional-dep')), true);
assert.equal(await fs.exists(path.join(config.cwd, 'node_modules', 'sub-failing')), false);
});
});

// Covers current behavior, issue opened whether this should be changed https://github.com/yarnpkg/yarn/issues/2274
test.concurrent('a subdependency of an optional dependency that fails should be installed',
(): Promise<void> => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
yarn-offline-mirror=./mirror-for-offline
Copy link
Member

Choose a reason for hiding this comment

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

Do you need offline mirror and .tgz files for the test?
I think you could be fine just using file: dependencies for tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think it's needed. I was trying to make the changes fit with their surroundings and mimic this test which uses a tgz file.

I'm happy to convert the new test to using file: dependencies. Should I also convert the one I modeled it after while I'm at it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's make it simpler in both places.
.tgz file are harder to review

Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"optionalDependencies": {
"dep-optional": "1.0.0"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1
[email protected]:
version "1.0.0"
resolved dep-optional-1.0.0.tgz#c8cbcee6f23932e14898a2dd4c76e38b37d31531
dependencies:
dep-incompatible "1.0.0"

[email protected]:
version "1.0.0"
resolved dep-incompatible-1.0.0.tgz#f711458d716eeb5c2cbbc11ee09edccedbb79e74
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
process.exit(0);
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "optional-failing",
"version": "0.0.0",
"scripts": {
"install": "node install.js"
},
"dependencies": {
"sub-failing": "file:../sub-failing"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"optionalDependencies": {
"optional-dep": "file:optional-dep"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
process.exit(1);
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "sub-failing",
"version": "0.0.0",
"scripts": {
"install": "node install.js"
}
}
3 changes: 2 additions & 1 deletion src/package-request.js
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ export default class PackageRequest {
promises.push(this.resolver.find({
pattern: depPattern,
registry: remote.registry,
optional: false,
// dependencies of optional dependencies should themselves be optional
optional: this.optional,
Copy link
Member

Choose a reason for hiding this comment

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

What happens if a dependency is optional in one subtree and is not optional in another:

A -> (optional) B -> C
D -> C

Would it mark C as optional even though D requires it as a regular dependency?
Could you add a test for that to make sure we don't break it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure that would work fine because of this code but I'll add a unit test to prove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm...couldn't your scenario be simplified to

(optional) A -> C
B -> C

and still prove what you are concerned about? I'm not seeing why the first chain has to have a depth of 3 -- am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Just a gut feeling, we had problems with deeper chains of dependency :)
Probably 2 layers should be good enough

parentRequest: this,
}));
}
Expand Down