Ignore optional dependency if wrong platform

[pierrefourgeaud]

#1435

Expect to solve the issue where optional dependencies' dependencies are not ignored as they should be.

---

What is the current behavior?

I get an error while using yarn install instead of just ignoring the dependency.

If the current behavior is a bug, please provide the steps to reproduce.

Here is an excerpt of my package.json:

{
  // ...
  "dependencies": {
    // ...
  },
  "devDependencies": {
    // ...
  },
  "optionalDependencies": {
    "appdmg": "^0.4.5",
    // ...
  },
  // ...
}

Then :
yarn install

Result:

yarn install v0.16.1
info No lockfile found.
warning [email protected]: No license field
[1/4] Resolving packages...
warning electron-prebuilt > electron-download > nugget > progress-stream > through2 > xtend > [email protected]: 
[2/4] Fetching packages...
warning [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
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.

As a comment, it is important to underline that appdmg has actually a dependency named ds-store that has one named macos-alias.

What is the expected behavior?

Yarn should completely ignore appdmg, therefore any dependency related to it.

Please mention your node.js, yarn and operating system version.
Node: 6.5.0
OS: Linux Ubuntu 16.04 (work on OSX 10.10.5 since appdmg is for Darwin).

[sebmck]

This exclusion already happens in package-compatibility.js, can you please explain more about the issue you're running into

[pierrefourgeaud]

I saw that too but still there is one issue left:

Yarn will not try to install that module especially (here appdmg) but it will still try to install the dependency of that module (ds-store) which might not be compatible with the platform either.

Trying to install a package with appdmg in optional dependency on Linux will fail miserably...

[pierrefourgeaud]

And the test are failing but I am investigating but I think the tests might not be right.

Installing fsevents on a Linux machine should be ignored (if marked as optional). But then we should not try to install nan and node-pre-gyp that are both dependencies of fsevents. There is no point in doing so.

[bestander]

@pierrefourgeaud, ping regarding the broken tests.

[pierrefourgeaud]

@bestander I haven't got the time to look into it. I can do that this week hopefully.

[mvestergaard]

I believe #1997 should resolve this, please have a look

[pierrefourgeaud]

I will check that right now. Thanks for the heads up. Is it in 0.18.0 ?

[pierrefourgeaud]

@mvestergaard After testing on 0.18.0 with MSYS on Windows the error still happen:

[mvestergaard]

Create an issue I suppose. There's been a bunch of other changes since my PR.

[bestander]

I think this was fixed in some other commit

[bendemboski]

@bestander this is definitely not fixed. #1435 is still open, and a number of bugs in other projects such as this and this appear to be caused by it.

You can easily repro it on linux:

$ mkdir 1438
$ cd 1438
$ yarn init -y
yarn init v0.19.1
warning The yes flag has been set. This will automatically answer yes to all questions which may have security implications.
success Saved package.json
Done in 0.09s.
$ yarn add electron-installer-dmg
yarn add v0.19.1
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
warning [email protected]: The platform "linux" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
error [email protected]: The platform "linux" is incompatible with this module.
error Found incompatible module
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.

It's a hard blocker for being able to use yarn with electron-forge, or I imagine any project with an optional dependency that has a dependency that isn't compatible with all OS's.

[bestander]

Ok, let's rebase it and merge it then.
@pierrefourgeaud, could you rebase and add a test to __tests__/commands/add.js please?
It could be excluded for MacOS builds

[bestander]

@bendemboski if you want to get it merged fast, in case PR author is busy, would you want take over the PR?

[bendemboski]

Sure, I can give it a shot.

[bendemboski]

@bestander #2811

[bestander]

Replaced with #2811