Fix: incorrect optional ignore in nested dependencies by BYK · Pull Request #4448 · yarnpkg/yarn

BYK · 2017-09-13T23:22:25Z

Summary

Fixes #4445. The bug was introduced in
https://github.com/yarnpkg/yarn/pull/3976/files#diff-7066979f95168f7bb59e4f9cfb3ba8fcR211
with an incomplete package optional check. This caused many transient
dependencies to be ignored at linking stage when --ignore-optional
flag is passed. This patch fixes the check to combine the flag with
the package's actual optional mark.

Test plan

Manually verified #4445 is resolved. Also added a unit test.

**Summary** Fixes #4445. The bug was introduced in https://github.com/yarnpkg/yarn/pull/3976/files#diff-7066979f95168f7bb59e4f9cfb3ba8fcR211 with an incomplete package optional check, causing many transient dependencies to be ignored at linking stage when `--ignore-optional` flag is passed. This patch fixes the check to combine the flag with the package's actual optional mark. **Test plan** Manually verified #4445 is resolved. Also added a unit test.

BYK · 2017-09-13T23:22:59Z

Also added a unit test.

This is not true yet, obviously. I will add a unit test soon and won't merge without tests for this.

BYK · 2017-09-14T01:24:14Z

src/package-request.js

    invariant(ref, 'Resolved package info has no package reference');
    ref.addRequest(this);
    ref.addPattern(this.pattern, resolved);
+    ref.addOptional(this.optional);


The new test case revealed another bug here: if a package is listed as optional by one package and as a strict dependency by another, it should be non-optional at the end. However, due to incorrect request caching below and the lack of this line here, it's optional status was random :(

nice catch!

bestander

Looks reasonable.
If tests are passing I am fine with it.

I would suggest adding a separate test case for this specific issue instead of making existing one bigger

BYK · 2017-09-14T02:05:58Z

If tests are passing I am fine with it.
I would suggest adding a separate test case for this specific issue instead of making existing one bigger

I was only able to find the additional issues when writing tests. Are you suggesting that I split this into two tests one with nested optional dependencies and keep the old one in addition to them?

kaylie-alexa · 2017-09-14T02:09:11Z

src/package-request.js

    invariant(ref, 'Resolved package info has no package reference');
    ref.addRequest(this);
    ref.addPattern(this.pattern, resolved);
+    ref.addOptional(this.optional);


nice catch!

kaylie-alexa · 2017-09-14T02:09:52Z

src/package-resolver.js

      return;
    } else {
-      this.fetchingPatterns[fetchKey] = true;
+      this.fetchingPatterns.add(fetchKey);


this else block isn't necessary since other wise it'll just return?

That's right, I'll fix it.

Cost me about 3 hours to catch it :D

kaylie-alexa · 2017-09-14T02:12:00Z

src/package-hoister.js

        }

-        const isMarkedAsOptional = !depinfo.pkg._reference || this.ignoreOptional;
+        const isMarkedAsOptional = depinfo.pkg._reference && depinfo.pkg._reference.optional && this.ignoreOptional;


sorry I deleted the previous comment because I realized the logic isn't quite the same. The previous one is saying mark as optional if depinfo.pkg._reference doesn't exist (or if ignoreOptional flag). That seemed a little weird to be but if that's still the case, we should probably restore it to the way you had it.

I know they are not equivalent but no tests failed and I feel like it makes more sense with the new version. What do you think?

Works for me. LET'S LIVE ON THE EDGE. 🎉

**Summary** Fixes yarnpkg#4445. The bug was introduced in https://github.com/yarnpkg/yarn/pull/3976/files#diff-7066979f95168f7bb59e4f9cfb3ba8fcR211 with an incomplete package optional check. This caused many transient dependencies to be ignored at linking stage when `--ignore-optional` flag is passed. This patch fixes the check to combine the flag with the package's actual optional mark. **Test plan** Manually verified yarnpkg#4445 is resolved. Also added a unit test.

acuper · 2017-12-07T11:07:01Z

Should the night build contain the fix ?
I installed it and still see missing dependencies errors while using --ignore-optional

BYK · 2017-12-07T11:16:04Z

@acuper this change went live with the 1.3.2 release

acuper · 2017-12-07T12:48:14Z

OK, I have 1.3.2, so for me --ignore-optional is still not working properly.
Will try to provide the steps how to reproduce but it might hard because this is happening in our internal projects where dependency hierarchy is huge.

Is there older version of yarn witch should has no the bug introduced inhttps://github.com//pull/3976/files#diff-7066979f95168f7bb59e4f9cfb3ba8fcR211
?

acuper · 2017-12-07T13:37:36Z

I think I found a way to reproduce it:

{ "dependencies": { "internal-package": "^4.1.0", "yargs": "^9.0.1" } }
now internal-package has this as optionalDependency:
"yargs": "^7.0.2"

the result of yarn --ignore-optional is missing package: is-fullwidth-code-point
which dependency path is: yargs -> string-width -> is-fullwidth-code-point

BYK · 2017-12-07T14:27:17Z

@acuper can you please create a new issue so we can track and debug this properly. I'd love to fix of this is broken.

BYK requested review from arcanis, bestander and kaylie-alexa September 13, 2017 23:22

Add test & fix more issues

dcf56c9

BYK commented Sep 14, 2017

View reviewed changes

BYK mentioned this pull request Sep 14, 2017

Error: Cannot find module when using --ignore-optional #4340

Closed

Better optional check

74c21c8

bestander approved these changes Sep 14, 2017

View reviewed changes

BYK added 2 commits September 14, 2017 03:00

Merge branch 'master' into optional

62f1110

Fix lint

df2c225

kaylie-alexa reviewed Sep 14, 2017

View reviewed changes

BYK added 2 commits September 14, 2017 15:13

Merge branch 'master' into optional

9fc5dbd

Remove obsolete else block

f930295

kaylie-alexa approved these changes Sep 14, 2017

View reviewed changes

BYK merged commit ef8185b into master Sep 14, 2017

BYK deleted the optional branch September 14, 2017 16:30

t-sauer mentioned this pull request Sep 14, 2017

Issue creating new project with yarn 1.0.1 glimmerjs/glimmer-blueprint#63

Closed

acuper mentioned this pull request Dec 7, 2017

Dependency is missing when using yarn --ignore-optional #5054

Closed

Conversation

BYK commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BYK commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander left a comment

Choose a reason for hiding this comment

Uh oh!

BYK commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acuper commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BYK commented Dec 7, 2017

Uh oh!

acuper commented Dec 7, 2017

Uh oh!

acuper commented Dec 7, 2017

Uh oh!

BYK commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BYK commented Sep 13, 2017 •

edited

Loading

acuper commented Dec 7, 2017 •

edited

Loading