Properly skip installation of packages that do not need to be installed (incompatible optionalDependencies for example)

[mvestergaard]

Summary

This is a different approach to #1438
It should make sure that packages incompatible with the current environment are not installed.
See #1954 for further motivation

With this change, any optionalDependencies that is incompatible with the environment will be skipped entirely in the install.

Installing a package.json like this on windows or linux:

{
  "optionalDependencies": {
    "fsevents": "*"
  }
}

Will result in an empty node_modules folder, which is the same behavior as using npm.

Test plan

The current test 'root install with optional deps' should test the scenario on anything other than OSX. It uses fsevents which is only compatible with OSX.
To test on OSX you need to modify __tests__/fixtures/install/root-install-with-optional-dependency/package.json to use an optional package that is not compatible with OSX.
CI servers are running linux, so they should be fine.
After running install on a package with a single optional incompatible dependency, node_modules should be empty.

Fixes #1059 #1435

[bestander]

@mvestergaard would you please add some tests as in your test plan?
We need to make sure that this feature does not break in the future and it is easy to break.
We have platform specific tests for install operation already.

[bestander]

Why is fetch true for a check operation?

[mvestergaard]

It's for this bit:

await this.fetcher.init();
await this.compatibility.init();

It seems that if the fetcher is not run beforehand, the compatibility check does not have the os and cpu data for each package to properly do the check.

[bestander]

Would this work offline?
I am afraid that initing fetcher may cause a longer check

[mvestergaard]

That's a good question. I mean if the package is in the cache, it doesn't actually fetch anything, right?

[mvestergaard]

This happens if the fetch is not a part of the check:

yarn check v0.18.0-0
error chokidar#fsevents not installed
error fsevents#nan not installed
error fsevents#node-pre-gyp not installed
error node-pre-gyp#npmlog not installed
error node-pre-gyp#rc not installed
error node-pre-gyp#tar-pack not installed
error rc#deep-extend not installed
error tar-pack#fstream-ignore not installed
error rc#minimist not installed
error tar-pack#once not installed
error tar-pack#uid-number not installed
error Found 11 errors.

Because the check does not exclude those package due to incompatibility, it expects them to be there, which it shouldn't.

[bestander]

Ah, it makes sense.
For cross platform modules - they are installed in node_modules and could be verified in place but the missing ones could be either "missing" or not installed because they are "optional".

Adding "optional" property to lockfile makes more sense but this needs a discussion involving more people, maybe an RFC?

[bestander]

As a matter of fact, I think there should be a third mode for check - #1934.
I would rather rely on that and the integrity check than the default one

[mvestergaard]

I don't think adding whether a package is optional to the lock file is enough alone.
You still want it to be installed on an OS where it is supported, but skipped on an OS where it isn't.

[bestander]

I wonder, if package is optional but it failed to install because you don't have a compiler, is it an error or is it ok?

[mvestergaard]

That should be an error.
In my opinion, if it is determined by the OS and CPU requirements of the package that your system is compatible, then any installation error that occurs, are out of the scope of what yarn should handle.

[mvestergaard]

@bestander I'll look into adding some tests tonight.
I suppose it kind of needs to be able to catch any issue on all OS?
Relying on fsevents alone will only catch issues on windows and linux, so something alternative has to be done to test on OSX.

[bestander]

I suppose it kind of needs to be able to catch any issue on all OS?
Relying on fsevents alone will only catch issues on windows and linux, so something alternative has to be done to test on OSX.

You could depend on a fake optional dependency that does not work on any OS.
In install command we use quite a few and save them in the test case' offline morror

[mvestergaard]

@bestander How does one go about correctly generating one of those .tgz files found in the mirror-for-offline fixture folders?

[bestander]

@bestander How does one go about correctly generating one of those .tgz files found in the mirror-for-offline fixture folders?

It is a bit involved.
Just run npm pack on a folder with package.json.
Then you'll need to manually edit yarn.lock file to add sha1 of the file, you could get it with some console.log in tarball-fetcher.js or use a sha1 util on Mac/linux.

[mvestergaard]

@bestander I added two tests.
The dep-incompatible package has this in it's package.json "os": [ "invalid" ] so it is considered incompatible on any OS.
It also has a dependency of dep-a. The second test verifies that dep-a is also skipped.

[bestander]

Great job, @mvestergaard, I'll have a look

[mvestergaard]

@bestander Any ETA on next release?

[bestander]

Saturday-Sunday this week

[friosl]

Several years later, this worked for me (just adding the "*" to fsEvent in optional packages), I was trying to do CI in aws and the problem was that package :D