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.

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

Bug: "resolutions" trumps --ignore-optional #6040

Closed
joscha opened this issue Jun 27, 2018 · 15 comments · Fixed by #7273
Closed

Bug: "resolutions" trumps --ignore-optional #6040

joscha opened this issue Jun 27, 2018 · 15 comments · Fixed by #7273

Comments

@joscha
Copy link

joscha commented Jun 27, 2018

Do you want to request a feature or report a bug?

bug

What is the current behavior?

Currently, when there are resolutions defined, --ignore-optional is ignored and the optional dependencies are installed anyway.

You can test it with a simple package.json:

{
  "name": "test-optional",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "optionalDependencies": {
    "fsevents": "^1.0.0"
  },
  "resolutions": {
    "fsevents": "^1.2.4"
  }
}

on yarn install --ignore-optional the above will lead to fsevents being installed nonetheless.

On Linux (Ubuntu Xenial) for example, this would fail the yarn install due to:

error [email protected]: The platform "linux" is incompatible with this module.
error Found incompatible module
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

yarn install --ignore-optional works as expected with:

{
  "name": "test-optional",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "optionalDependencies": {
    "fsevents": "^1.0.0"
  }
}

and does not install fsevents.

What is the expected behavior?

No optional dependencies to be installed with --ignore-optional, no matter whether they are mentioned in resolutions or not.

Please mention your node.js, yarn and operating system version.

yarn 1.7.0
Node 10.5.0
OSX High Sierra and Debian Xenial

@Gudahtt
Copy link
Member

Gudahtt commented Aug 19, 2018

I was able to reproduce this problem. If fsevents is an optional dependency (direct or transitive) and is specified in resolutions, the install fails on non-macOS platforms. It even fails without the --ignore-optional flag.

It's not clear on what the expected behavior is for optional dependencies specified in resolutions. Intuitively I'd expect the version set in resolutions to be ignored if the optional package fails to install, but I can't find that specified in the selective-versions-resolutions RFC.

@deftomat
Copy link

deftomat commented Aug 23, 2018

The same for me with yarn 1.9.4.
I'm pretty sure that the expected behaviour for resolutions is JUST to override a package version. Not to force their installation. Current behaviour is unusable when you have developers with different environments 😞

@wycats
Copy link
Member

wycats commented Nov 8, 2018

I hit this problem too, and agree that this behavior is unexpected. Is there even a workaround?

@rally25rs
Copy link
Contributor

rally25rs commented Nov 9, 2018

Took some time to start digging into this. Trying to understand exactly how resolutions works, but it looks like this line https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L286 makes a pkg that is hard-coded to optional: false which is then used to replace another dependency when it matches the resolution.

This happens before the actual dependencies are matched to resolutions, so the above line can't just be altered to match the actual dependency's optional property.

I haven't found the exact spot yet, but I'm guessing that at some point it's taking the fsevents@^1.0.0 optional: true reference and replacing it with the already-resolved fsevents@^1.2.4 optional: false.

If anyone has time to jump in and help, it would be appreciated...

@jasonxia23
Copy link

Would like to take a try.

parris added a commit to storyforj/fervor that referenced this issue Feb 5, 2019
yarnpkg/yarn#6040

Version resolutions reverse the optional'ness of a dependency.
Workaround is the install with the resolution on a compatible
environment then remove the resolution.
@parris
Copy link

parris commented Feb 5, 2019

I think I found a possible workaround (that may work in some cases - worked for me with chokidar and fsevents) - install with the package's version resolution on a compatible environment. Then remove the resolution and yarn install again. Commit all that. The only problem with this is needing to redo the resolution from time to time.

@ghost
Copy link

ghost commented May 2, 2019

@joscha I have similar problem. Did you find any good work around for it?

@joscha
Copy link
Author

joscha commented May 3, 2019

@joscha I have similar problem. Did you find any good work around for it?

I did not unfortunately, sorry.

@coreyward
Copy link

I suspect it has something to do with line 289, where optional: false is hardcoded:

for (const packageName of Object.keys(this.resolutionMap.resolutionsByPackage)) {
for (const {pattern} of this.resolutionMap.resolutionsByPackage[packageName]) {
resolutionDeps = [...resolutionDeps, {registry, pattern, optional: false, hint: 'resolution'}];
}
}

This behavior was introduced by @kaylie-alexa (committed by @arcanis) in #4105. Perhaps one of them can shed some light on how to proceed with allowing resolutions to co-exist with optional dependencies?

@kaylie-alexa
Copy link
Member

kaylie-alexa commented May 14, 2019

So I looked into the issue, and the real problem is that the deduping function is grouping the two dependency types together if their semver ranges match. So for example if you have

 {
  "name": "test-optional",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "optionalDependencies": {
    "fsevents": "1.0.0"
  },
  "resolutions": {
    "fsevents": "^1.2.4" // resolves to 1.2.9
}

you'll notice that the --ignore-optional flag will work as intended. I'll look into an actual fix, but that'd be the temporary workaround.

@olingern
Copy link
Contributor

olingern commented May 14, 2019

@kaylie-alexa I started looking at this, too. Maybe we can compare notes. Here's where I'm at:

  • Checking validity via removing and adding resolutions in a test project. ( slightly painful 😢 )
  • Logging my tears away

Basically, I'm just:

  • rm -rf ./node_modules
  • Running ./bin/yarn --ignore-optional
  • Isolating differences between a package.json with resolutions and without.

With this package.json:

{
  "name": "yarn_testing",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "optionalDependencies": {
    "left-pad": "^1.3.0"
  },
  "author": "",
  "license": "ISC",
  "dependencies": {
    "right-pad": "^1.0.1"
  },
  "resolutions": {
    "left-pad": "^1.3.0"
  }
}

Fix

Ensure resolutions do not conflict with optional dependencies

I was able to verify that resolutions make a package required, so even when --ignore-optional is specified, you'll see optional dependencies marked as required in visibleFlatTree that's returned from PackageHoister.init, which is used to install dependencies. So, when a resolution is specified you get something like this:

    HoistManifest {
      isDirectRequire: true,
      isRequired: true,
      isIncompatible: false,
      loc: '/home/olingern/.cache/yarn/v4/npm-left-pad-1.3.0-5b8a3a7765dfe001261dde915589e782f8c94d1e/node_modules/left-pad',
      pkg: [Object],
      key: 'left-pad',
      parts: [Array],
      originalKey: 'left-pad',
      previousPaths: [],
      history: [Array],
      isNohoist: false,
      originalParentPath: '',
      shallowPaths: [],
      isShallow: false,
      nohoistList: null } ],

isRequired for the package left-pad should be false!

Digging into isRequired

Getting to it was tricky, but with some pain I tracked it down:

Some debugging output for good measures around this line. Here's the debug code I used:

    console.log("\n")
    console.log("--------------------------------------")
    console.log(pattern + " isRequired --> " + isRequired)
    console.log("--------------------------------------")
    console.log("ref.optional \t\t | \t" + ref.optional);
    console.log("this.ignoreOptional \t | \t" + this.ignoreOptional);
    console.log("isDirectRequire \t | \t" + isDirectRequire);
    console.log("!ref.ignore \t\t | \t" + !ref.ignore);
    console.log("!isIncompatible \t | \t" + !ref.ignore);
    console.log("!isMarkedAsOptional \t | \t" + !isMarkedAsOptional);

No resolutions:

--------------------------------------
right-pad@^1.0.1 isRequired --> true
--------------------------------------
ref.optional             |      false
this.ignoreOptional      |      true
isDirectRequire          |      true
!ref.ignore              |      true
!isIncompatible          |      true
!isMarkedAsOptional      |      true


--------------------------------------
left-pad@^1.3.0 isRequired --> false
--------------------------------------
ref.optional             |      true   <-- correct
this.ignoreOptional      |      true
isDirectRequire          |      true
!ref.ignore              |      true
!isIncompatible          |      true
!isMarkedAsOptional      |      true

With resolutions:

--------------------------------------
right-pad@^1.0.1 isRequired --> true
--------------------------------------
ref.optional             |      false
this.ignoreOptional      |      true
isDirectRequire          |      true
!ref.ignore              |      true
!isIncompatible          |      true
!isMarkedAsOptional      |      true


--------------------------------------
left-pad@^1.3.0 isRequired --> true
--------------------------------------
ref.optional             |      false   <-- not correct
this.ignoreOptional      |      true
isDirectRequire          |      true
!ref.ignore              |      true
!isIncompatible          |      true
!isMarkedAsOptional      |      true

Eventually, I arrived back at exactly where @coreyward points out that this could be a problem. It's due to actually returning two entries of the same package, one optional and one not.

    return {
      requests: [...resolutionDeps, ...deps],
      patterns,
      manifest,
      usedPatterns,
      ignorePatterns,
      workspaceLayout,
    };

If we log what is being returned here, we'll find this:

=====================
  resolutionDeps
=====================
[ { registry: 'npm',
    pattern: 'left-pad@^1.3.0',
    optional: false,
    hint: 'resolution' } ]
=====================
  deps
=====================
[ { pattern: 'right-pad@^1.0.1',
    registry: 'npm',
    hint: null,
    optional: false,
    workspaceName: 'yarn_testing',
    workspaceLoc: undefined },
  { pattern: 'left-pad@^1.3.0',
    registry: 'npm',
    hint: 'optional',
    optional: true,
    workspaceName: 'yarn_testing',
    workspaceLoc: undefined } ]

PR

I opened #7272 as a place to talk about the implementation.

@kaylie-alexa
Copy link
Member

Thanks so much for sleuthing @olingern! You're right resolutions shouldn't always default to being optional. I posted a PR to address the issue.

@xMort
Copy link

xMort commented Mar 24, 2020

When was this released? I just encountered this behavior with fsevents and yarn 1.22.4

@seanmakesgames
Copy link

seanmakesgames commented May 4, 2020

Took a look at the fix and it looks like the 'checking for if it's marked as optional' is happening at the same package, not looking at the fully resolved subdependencies' packages optional status
b656953#diff-ffb8cf10e4b2d16cc7df603574698af6R289
If you aren't marking the dependency as optional in the top level package.
Additionally, you must specify --ignore-optional to get it to work.

Any of you thumbs-uppers from the comment above (march 24th) in the same situation as us?

@bdurrer
Copy link

bdurrer commented Sep 21, 2020

@seanmakesgames Sadly yes, I get the error with yarn 1.22.5 on windows using the configuration below, unless I specifiy --ignore-optional. (Several versions of fsevents 1.x.x are referenced by sub-dependencies too)

  "optionalDependencies": {
    "fsevents": "1.2.13"
  },
  "resolutions": {
    "fsevents": "1.2.13",
    "**/fsevents": "1.2.13"
  },

eps1lon added a commit to eps1lon/react that referenced this issue Feb 15, 2024
eps1lon added a commit to eps1lon/react that referenced this issue Feb 16, 2024
eps1lon added a commit to eps1lon/react that referenced this issue Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet