fix(linking): Handle missing package when using --ignore-optional #5059

kimjoar · 2017-12-08T11:49:20Z

Summary

Closes #5054, closes #4876, closes #5152.

Currently when running yarn --ignore-optional required dependencies can be marked as optional because they exist in the tree of optionalDependencies of one of the dependencies (but it's required by some other non-optional dependency).

For example: once depends on wrappy, but wrappy is also in the chain from fsevents (glob -> inflight -> wrappy) which is an optional dependency of chokidar. So if the first wrappy comes from an "optional chain" when yarn processes it, it ends up being marked as _reference.optional = true, and the package hoister therefore doesn't mark it as required.

When running the released version of yarn with https://github.com/spalger/reproduce-issues/blob/master/yarn-ignores-non-optional-dependencies you'll see

~/dev/reproduce-issues/yarn-ignores-non-optional-dependencies
$ yarn --ignore-optional
yarn install v1.3.2-20171204.1856
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
✨  Done in 0.17s.

~/dev/reproduce-issues/yarn-ignores-non-optional-dependencies
$ yarn check --verify-tree
yarn check v1.3.2-20171204.1856
error "once#wrappy" not installed
error An unexpected error occurred: "Found 1 errors.".

This fix checks whether or not the parent is marked as required, and if so, marking the dependency as required unless it's listed in the parent's optionalDependencies.

Test plan

I tested this implementation with both https://github.com/spalger/reproduce-issues/blob/master/yarn-ignores-non-optional-dependencies and #4876, and in both cases yarn check --verify-tree succeeded after running yarn --ignore-optional.

Also added automated tests.

kaylie-alexa · 2017-12-08T20:31:33Z

Woot! Thanks @kjbekkelund, do you feel comfortable trying to resolve this bug? Otherwise I can take a look it. Let me know :)

kimjoar · 2017-12-08T22:06:14Z

@kaylieEB I've been exploring a bit, but I haven't had much progress yet. Would be great if you could take a look.

kimjoar · 2017-12-08T22:48:44Z

I think it's related to resolveToExistingVersion. In this case, when it resolves once to the one that already exists it switches optional from true to false, but it doesn't walk its dependencies.

So it feels like there's a need to walk the dependencies in addOptional (

yarn/src/package-reference.js

Line 115 in 9c2bbca

this.optional = false;

) and switch it for them too.

Thoughts?

kimjoar · 2017-12-08T23:51:20Z

Ah, or maybe _propagateRequired is the location for a fix?

yarn/src/package-hoister.js

Line 221 in ef8185b

_propagateRequired() {

However, that depends on that _reference.optional value again, which is true instead of false here for wrappy

buildsize · 2017-12-09T00:47:10Z

This change will increase the build size from 10.51 MB to 10.51 MB, an increase of 762 bytes (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	910.44 KB	910.48 KB	38 bytes (0%)
yarn-[version].js	3.96 MB	3.96 MB	354 bytes (0%)
yarn-legacy-[version].js	4.11 MB	4.11 MB	352 bytes (0%)
yarn-v[version].tar.gz	915.84 KB	916.06 KB	226 bytes (0%)
yarn_[version]all.deb	676.22 KB	676.02 KB	-208 bytes (0%)

kimjoar · 2017-12-09T00:53:53Z

@kaylieEB Looks like I got it running in the end.

The problem is that you need to know if it was listed in optionalDependencies when deciding whether or not to make them required. To fix this I re-used the hint field, which is already used in this way in e.g.

yarn/src/cli/commands/install.js

Lines 319 to 321 in 2065988

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

Let me know if there's a better way to handle this.

kimjoar · 2017-12-10T19:27:48Z

With the latest fix, this should also handle #4876

BYK

Looks pretty good, thank you!

I've added some comments and curious what you think about those suggestions.

BYK · 2017-12-13T16:57:07Z

src/package-hoister.js

-        const isMarkedAsOptional = depinfo.pkg._reference && depinfo.pkg._reference.optional && this.ignoreOptional;
+        const depRef = depinfo.pkg._reference;
+
+        let isMarkedAsOptional = depRef && depRef.optional && this.ignoreOptional;


We should be able to reduce this to:

let isMarkedAsOptional = depRef && depRef.optional && this.ignoreOptional && !(info.isRequired && depRef.hint !== 'optional');

What do you think? The comment you added should stay though, it is great!

BYK · 2017-12-13T16:59:46Z

src/package-request.js

@@ -52,6 +53,7 @@ export default class PackageRequest {
  config: Config;
  registry: ResolverRegistryNames;
  optional: boolean;
+  hint: ?string;


Would you mind changing this to an enum both to avoid unnecessary string usage all over the app and to avoid potential typing errors.

My suggestion is to add something like the following into constants.js and use that:

const REQUEST_HINTS = Object.freeze({ dev: Symbol('REQUEST_HINT_DEV'), optional: Symbol('REQUEST_HINT_OPTIONAL'), });

BYK · 2017-12-13T17:00:38Z

src/package-hoister.js

+        // If it's marked as optional, but the parent is required and the
+        // dependency was not listed in `optionalDependencies`, then we mark the
+        // dependency as required.
+        if (isMarkedAsOptional && info.isRequired && depRef && depRef.hint !== 'optional') {


Can we not use depRef.isRequired instead of this? Or may be add depRef.isOptional?

I tried that at first, but it fails with "deeper optionals". E.g.

foo (in `optionalDependencies`) -> bar -> baz quux -> bar -> baz

Here both bar and baz will have _reference.isOptional === true because of foo. However, as this code doesn't mark the reference when performing this logic (only setting depinfo.isRequired = true), baz will stay optional as bar._reference.optional === true. If we changed the logic to also do a depRef.setOptional(false) in addition to setting depinfo.isRequired this approach would work. That was my initial fix, but I changed to this approach as I wasn't sure about doing that work from the package hoister. Thoughts?

kimjoar · 2017-12-14T11:54:39Z

@BYK I ended up doing:

export type RequestHint = 'dev' | 'optional' | 'resolution' | 'workspaces';

as this field seems to be used for a couple things. That's likely a sign this should be cleaned up(?) Given

yarn/src/cli/commands/outdated.js

Line 33 in 19c8cd5

    
           const getNameFromHint = hint => (hint ? `${hint}Dependencies` : 'dependencies');

it definitely feels like the intention was to codify what type of dependency it is.

Thoughts? I'd of course prefer to not extend the scope of this PR ;)

kimjoar · 2018-01-04T21:40:06Z

@BYK Just back from my holidays, so I thought I'd ping you about my suggestion above.

BYK · 2018-01-05T17:05:40Z

@kjbekkelund I think the PR is fine but I have realized this is masking another issue which makes this part of the code as complex as it is right now which is marking all transient dependencies of optional dependencies as optional too, opening the door for potentially corrupt installations for optional dependencies.

Also we apparently already have a test case for this behavior:

yarn/__tests__/commands/install/integration.js

Lines 956 to 967 in 3e1c3a7

    
           test.concurrent('a sub-dependency should be non-optional if any parents mark it non-optional', async (): Promise< 
        
             void, 
        
           > => { 
        
             let thrown = false; 
        
             try { 
        
               await runInstall({}, 'failing-sub-dep-optional-and-normal', () => {}); 
        
             } catch (err) { 
        
               thrown = true; 
        
               expect(err.message).toContain('sub-failing: Command failed'); 
        
             } 
        
             expect(thrown).toEqual(true); 
        
           });

May be instead of adding all these new files, you can fix that test to cover transient dependencies (or just extend it).

What do you think?

Ember CLI detects the yarn.lock file to decide whether to use Yarn or not. Pending yarnpkg/yarn#5059, we should switch to npm by default.

kimjoar · 2018-03-02T16:19:16Z

@BYK Finally had some time to get this done. Are you okey with this approach? Is the fs.readdir in the test okey, or do you want me to list separate fs.exists assertions? (as this ends up mentioning the integrity file)

BYK

Hey! This new version looks solid. I think fs.readdir approach is actually neat and makes it more readable but I'll see what @arcanis thinks before merging.

Thanks a lot for coming back and pushing this forward! ❤️

BYK · 2018-03-07T11:59:40Z

src/cli/commands/install.js

+      const pushDeps = (
+        depType,
+        manifest: Object,
+        {hint, optional}: {hint: ?constants.RequestHint, optional: boolean},


Why make hint optional here?

Because of

yarn/src/cli/commands/install.js

Line 327 in 5ef113f

pushDeps('dependencies', projectManifestJson, {hint: null, optional: false}, true);

Not sure if constants.RequestHint | null or ?constants.RequestHint is preferred(?) Or maybe add a production hint and always require hint?

BYK · 2018-03-07T12:01:29Z

src/constants.js

@@ -124,3 +124,5 @@ export const VERSION_COLOR_SCHEME: {[key: string]: VersionColor} = {
 };

 export type VersionColor = 'red' | 'yellow' | 'green' | 'white';
+
+export type RequestHint = 'dev' | 'optional' | 'resolution' | 'workspaces';


Don't see other hints being used anywhere else. Do we need anything other than 'dev' and 'optional' at the moment?

resolution is used in:

yarn/src/cli/commands/install.js

Line 281 in 5ef113f

resolutionDeps = [...resolutionDeps, {registry, pattern, optional: false, hint: 'resolution'}];

and workspaces in:

yarn/src/cli/commands/install.js

Line 379 in 5ef113f

pushDeps('workspaces', {workspaces: virtualDep}, {hint: 'workspaces', optional: false}, true);

BYK · 2018-03-07T17:32:47Z

I'll merge this since it fixes an important issue but I'd love to get answers to my final comments and maybe get a follow-up to address them.

This option is apparently fixed in yarn 1.6.0: yarnpkg/yarn#5059

kimjoar mentioned this pull request Dec 8, 2017

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

Closed

kimjoar force-pushed the bug/ignoring-too-many-deps branch from c2c64ac to 6ad221f Compare December 9, 2017 00:43

kimjoar mentioned this pull request Dec 9, 2017

Switch to Yarn elastic/kibana#15485

Merged

kimjoar changed the title ~~Test that fails because of missing deps after install~~ Handle missing package when using --ignore-optional Dec 10, 2017

kimjoar force-pushed the bug/ignoring-too-many-deps branch 2 times, most recently from 16eee4d to 6437382 Compare December 10, 2017 19:24

kimjoar mentioned this pull request Dec 10, 2017

--ignore-optional is broken #4876

Closed

kimjoar added 2 commits December 11, 2017 23:04

Test that fails because of missing deps after install

0d996fb

Potential solution

70e5d5f

kimjoar force-pushed the bug/ignoring-too-many-deps branch from 6437382 to 39bc4ee Compare December 11, 2017 22:04

Improved test case and implementation

abbd567

kimjoar force-pushed the bug/ignoring-too-many-deps branch from 39bc4ee to abbd567 Compare December 12, 2017 17:41

BYK requested changes Dec 13, 2017

View reviewed changes

kimjoar added 2 commits December 13, 2017 20:44

Review feedback, isMarkedAsOptional

f847983

Type request hints

474be64

Merge branch 'master' into bug/ignoring-too-many-deps

231ca5e

This was referenced Jan 10, 2018

Switch to released version of Yarn elastic/kibana#15952

Closed

Doesn't install correct dependencies for 1.3.2 #5152

Closed

tvald mentioned this pull request Jan 19, 2018

[FR] a way to ignore packages #4611

Open

tomdale mentioned this pull request Feb 1, 2018

Install optional dependencies when creating a new project ember-cli/ember-cli#7594

Merged

tomdale added a commit to glimmerjs/glimmer-blueprint that referenced this pull request Feb 2, 2018

Remove Yarn

7c8f66a

Ember CLI detects the yarn.lock file to decide whether to use Yarn or not. Pending yarnpkg/yarn#5059, we should switch to npm by default.

tomdale added a commit to glimmerjs/glimmer.js that referenced this pull request Feb 6, 2018

Remove Yarn

d8f79a8

Ember CLI detects the yarn.lock file to decide whether to use Yarn or not. Pending yarnpkg/yarn#5059, we should switch to npm by default.

kimjoar added 2 commits March 2, 2018 06:56

Merge branch 'master' into bug/ignoring-too-many-deps

a661b2d

Improve test

c607be2

BYK approved these changes Mar 7, 2018

View reviewed changes

BYK reviewed Mar 7, 2018

View reviewed changes

BYK changed the title ~~Handle missing package when using --ignore-optional~~ fix(linking): Handle missing package when using --ignore-optional Mar 7, 2018

BYK merged commit 95b3dfe into yarnpkg:master Mar 7, 2018

kimjoar mentioned this pull request Apr 17, 2018

Move to Yarn 1.6.0 elastic/kibana#17741

Merged

jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Apr 27, 2018

Enable ignore-optional for yarn

9fc7638

This option is apparently fixed in yarn 1.6.0: yarnpkg/yarn#5059

jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Apr 27, 2018

Enable ignore-optional for yarn

d4a9db6

This option is apparently fixed in yarn 1.6.0: yarnpkg/yarn#5059

tylersmalley mentioned this pull request Jul 20, 2018

Update contributing guide to specify yarn version elastic/kibana#21039

Closed

blink1073 mentioned this pull request Aug 13, 2018

Build fails with Node 10 jupyterlab/jupyterlab#4585

Closed

blink1073 mentioned this pull request Aug 13, 2019

Cannot install any JupyterLab ExtensionRuntimeError: JupyterLab failed to build jupyterlab/jupyterlab#7003

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(linking): Handle missing package when using --ignore-optional #5059

fix(linking): Handle missing package when using --ignore-optional #5059

kimjoar commented Dec 8, 2017 •

edited by BYK

kaylie-alexa commented Dec 8, 2017

kimjoar commented Dec 8, 2017

kimjoar commented Dec 8, 2017 •

edited

kimjoar commented Dec 8, 2017

buildsize bot commented Dec 9, 2017 •

edited

kimjoar commented Dec 9, 2017 •

edited

kimjoar commented Dec 10, 2017

BYK left a comment

BYK Dec 13, 2017

BYK Dec 13, 2017

BYK Dec 13, 2017

kimjoar Dec 13, 2017 •

edited

kimjoar commented Dec 14, 2017

kimjoar commented Jan 4, 2018

BYK commented Jan 5, 2018

kimjoar commented Mar 2, 2018

BYK left a comment

BYK Mar 7, 2018

kimjoar Mar 8, 2018

BYK Mar 7, 2018

kimjoar Mar 8, 2018

BYK commented Mar 7, 2018

fix(linking): Handle missing package when using --ignore-optional #5059

fix(linking): Handle missing package when using --ignore-optional #5059

Conversation

kimjoar commented Dec 8, 2017 • edited by BYK

kaylie-alexa commented Dec 8, 2017

kimjoar commented Dec 8, 2017

kimjoar commented Dec 8, 2017 • edited

kimjoar commented Dec 8, 2017

buildsize bot commented Dec 9, 2017 • edited

kimjoar commented Dec 9, 2017 • edited

kimjoar commented Dec 10, 2017

BYK left a comment

Choose a reason for hiding this comment

BYK Dec 13, 2017

Choose a reason for hiding this comment

BYK Dec 13, 2017

Choose a reason for hiding this comment

BYK Dec 13, 2017

Choose a reason for hiding this comment

kimjoar Dec 13, 2017 • edited

Choose a reason for hiding this comment

kimjoar commented Dec 14, 2017

kimjoar commented Jan 4, 2018

BYK commented Jan 5, 2018

kimjoar commented Mar 2, 2018

BYK left a comment

Choose a reason for hiding this comment

BYK Mar 7, 2018

Choose a reason for hiding this comment

kimjoar Mar 8, 2018

Choose a reason for hiding this comment

BYK Mar 7, 2018

Choose a reason for hiding this comment

kimjoar Mar 8, 2018

Choose a reason for hiding this comment

BYK commented Mar 7, 2018

kimjoar commented Dec 8, 2017 •

edited by BYK

kimjoar commented Dec 8, 2017 •

edited

buildsize bot commented Dec 9, 2017 •

edited

kimjoar commented Dec 9, 2017 •

edited

kimjoar Dec 13, 2017 •

edited