[rush] Optional dependencies not handled properly

[alexghr]

When I try to use rush on a package that depends on webpack on a laptop that runs Linux I get the following error:

alexghr@alexghr-XPS-15-9560:~/Work/rushtest$ rush update


Rush Multi-Project Build Tool 5.0.0 - https://rushjs.io


Creating folder: /home/alexghr/Work/rushtest/common/config/rush
Starting "rush update"

Script is out of date; updating "/home/alexghr/Work/rushtest/common/scripts/install-run.js"
Script is out of date; updating "/home/alexghr/Work/rushtest/common/scripts/install-run-rush.js"

Trying to acquire lock for npm-6.2.0
Acquired lock for npm-6.2.0
Found npm version 6.2.0 in /home/alexghr/.rush/npm-6.2.0

Symlinking "/home/alexghr/Work/rushtest/common/temp/npm-local"
  --> "/home/alexghr/.rush/npm-6.2.0"

Updating temp projects in /home/alexghr/Work/rushtest/common/temp/projects
Updating /home/alexghr/Work/rushtest/common/temp/projects/foo.tgz
Finished creating temporary modules (0.01 seconds)

Checking node_modules in /home/alexghr/Work/rushtest/common/temp

Deleting the "npm-cache" folder
Deleting the "npm-tmp" folder

Running "npm install" in /home/alexghr/Work/rushtest/common/temp

npm notice created a lockfile as package-lock.json. You should commit this file.
added 339 packages from 277 contributors and audited 6807 packages in 7.946s
found 0 vulnerabilities


Running "npm shrinkwrap"...
npm notice package-lock.json has been renamed to npm-shrinkwrap.json. npm-shrinkwrap.json will be used for future installations.
"npm shrinkwrap" completed


Applied workaround for NPM 5 bug

Updating /home/alexghr/Work/rushtest/common/config/rush/npm-shrinkwrap.json

Linking projects together...

ERROR: Failed to parse package.json for fsevents: ENOENT: no such file or directory, open '/home/alexghr/Work/rushtest/common/temp/node_modules/fsevents/package.json'

The project depends (transitively, through webpack) on fsevents which is a package that can only be installed on a Mac.
The shrinkwrap file at common/config/rush/npm-shrinkwrap.json correctly marks fsevents as optional.

My minimal test case:

alexghr@alexghr-XPS-15-9560:~/Work/rushtest$ tree
.
|-- foo
|   `-- package.json
`-- rush.json

1 directory, 2 files

alexghr@alexghr-XPS-15-9560:~/Work/rushtest$ cat rush.json 
{
  "rushVersion": "5.0.0",
  "npmVersion": "6.2.0",
  "nodeSupportedVersionRange": ">=8.11.1 <9.0.0",

  "projects": [
    {
      "packageName": "foo",
      "projectFolder": "foo"
    }
  ]
}

alexghr@alexghr-XPS-15-9560:~/Work/rushtest$ cat foo/package.json 
{
  "name": "foo",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "devDependencies": {
          "webpack": "*"
  },
  "keywords": [],
  "author": "",
  "license": "ISC"
}

[octogonz]

Are you able to reproduce the issue using NPM 4.5.0? NPM 5.x and 6.x have various known bugs that sometimes cause trouble for Rush.

[alexghr]

Just tried it with npm 4.5.0 and it works fine!
It also works with pnpm 2.12.0 but webpack has other issues with the node_modules folder structure generated by pnpm that blocks me from using it pnpm/pnpm#801

[octogonz]

I need to update the documentation to recommend NPM 4.5.0. Longer term, we need to figure out what to do about these NPM issues. If they cannot be fixed, perhaps we should support Yarn instead of NPM.

[octogonz]

FYI Yarn is now supported as of Rush 5.1.0.

[factoidforrest]

I'm having an identical issue with yarn. The dependencies get skipped as optional, and then webpack throws on fsevents and a few other osx specific packages. Is there a way to force rush to install these?

I set ignore-optional=false in /common/config/rush/.npmrc. Didn't fix it.

I'm seeing

LINKING: api
Skipping optional dependency: dtrace-provider
Skipping optional dependency: mv
Skipping optional dependency: safe-json-stringify
Skipping optional dependency: dtrace-provider
Skipping optional dependency: fsevents
Skipping optional dependency: safe-json-stringify
Skipping optional dependency: bcrypt-pbkdf
Skipping optional dependency: ecc-jsbn
Skipping optional dependency: jsbn
Skipping optional dependency: tweetnacl

[octogonz]

In the old days, we encountered this problem: If I ran npm install on a Mac, the optional dependencies would get installed and thus be added to my shrinkwrap file. But if I ran npm install on a PC, because the optional dependencies weren't supported on Windows, they would not get installed, and NPM would NOT add them to the shrinkwrap file.

This meant that the contents of our shrinkwrap file depended on which OS was used to run rush update last. That is very confusing and caused problems. It seems like an incorrect design to me. So as a workaround, we specified --no-optional to always skip all dependencies. Maybe we lost some speed/features, but determinism/reliability are more important than speed or features.

Looking at the code, this flag seems to be added for PNPM and NPM but not Yarn. But then rush link never tries to link them, which is inconsistent in Yarn's case. We should fix that logic.

But first, I'm curious about what package managers are doing in the year 2018. If we're lucky, perhaps PNPM/NPM/Yarn have all evolved to have the correct design, i.e. including optional dependencies in the shrinkwrap file even if they were not installed. If so, then we could remove this workaround entirely. Do you happen to know the answer? :-P

[factoidforrest]

Yarn now obeys the skip optional dependencies command. It will read it out of the npmrc/yarnrc as well. https://yarnpkg.com/lang/en/docs/cli/install/#toc-yarn-install-ignore-optional

I understand why we wouldn't want them in rush's lock file, but they at least need to locally install so that webpack will run on OSX.

I'd argue that because determinism and reliability are more important, it's better to ALWAYS install optional dependencies even if they won't be used. That way it is the same everywhere. This webpack stuff won't even run on OSX without the "optional" modules.

[octogonz]

I understand why we wouldn't want them in rush's lock file, but they at least need to locally install so that webpack will run on OSX.

Something about your setup requires an optional dependency? How does that work? (We use webpack on both Mac/PC and we haven't had this issue. Usually the optional dependencies are just for extra features or optimizations.)

[factoidforrest]

Digging a little more, it looks like it's because of a package that declares its dependencies wrong. I submitted a fix request. TritonDataCenter/node-sshpk#53

In the mean time, wouldn't it be good if there were a way for rush to link optional dependencies?

[octogonz]

I just tried an experiment with the various package managers on a Windows PC: I installed dirgen which depends on fsevents which has "os": [ "darwin" ] that is incompatible with Windows. The results:

NPM 3.10.8

• Does NOT unzip the tarball into node_modules
• Does NOT write fsevents into npm-shrinkwrap.json at all
• Shows a warning about being optional

NPM 6.4.1

• Does NOT unzip the tarball into node_modules
• Writes the fsevents tarball URL into package-lock.json, marked as optional (in fsevents entry)
• Shows a warning about being optional

Yarn 1.10.1

• Does NOT unzip the tarball into node_modules
• Writes the fsevents tarball URL into yarn.lock, marked as optional (in dirgen entry)
• Shows a warning about being optional

PNPM 2.16.3:

• Unzips the tarball into node_modules (@zkochan FYI)
• Writes the fsevents tarball URL into shinkwrap.yaml, marked as optional (in dirgen entry)
• Does NOT show a warning about being optional

So old NPM releases are the only obstacle to eliminating --no-optional. The next step would be to figure out which version of NPM fixed the optionalDependencies behavior. If it's very old, we can simply issue an error saying that Rush 5 doesn't support that version. On the other hand, if NPM 4.5.0 is also broken, then I guess we'd need to preserve the workaround code based on a version check.

[octogonz]

NPM 4.5.0

• Does NOT unzip the tarball into node_modules
• Does NOT write fsevents into npm-shrinkwrap.json at all :'(
• Shows a warning about being optional

Darn...

[octogonz]

In the mean time, wouldn't it be good if there were a way for rush to link optional dependencies?

So I think what we should do is:

• Add --no-optional to the installer command line ONLY if it is NPM <5
• rush link should link optional dependencies if they are found on disk (i.e. if the package manager decided to install them)
• Perhaps it's still useful for rush link to tell us about dependencies that did not get linked because they are optional. But it should not be a yellow-colored warning, because that misleadingly implies something is wrong.

I marked this as "help wanted" because this is a pretty easy fix. But if nobody in the community volunteers, then I'll see if someone from our team can do it.

[elliottsj]

I'd like to work on this; can probably start working on a fix tomorrow or Sunday.

Looking forward to being able to run jest --watch on macOS out of the box again 😄

[elliottsj]

Sorry, I didn't get around to it on the weekend and probably won't have time during this week. Someone else can start tackling this if they like, otherwise I think I can take a look this coming weekend.

[elliottsj]

I got around to making the fix tonight: #878

The changes are fewer than I expected, so I feel like I'm missing something 😅

[octogonz]

Awesome! Thanks a lot for fixing this!

The changes are fewer than I expected, so I feel like I'm missing something 😅

Hmm... no, it looks reasonable to me. If you tested it and it works, that's good enough for me!

[jcgertig]

So i am hitting this issue but with latest pnpm. in my own package.json we have a optional dep that only for windows, but on windows it still does not install

[wasd171]

Same for OS X with pnpm, my use case is putting one dependency in optionalDependencies and manipulating the .npmrc:optional to exclude it from the installation for the Docker builds.

• The .npmrc setting does not seem to work at all, the package is always downloaded, but not always linked correctly
• Putting the dependency into the optionalDependencies downloads it to the common/temp/node_modules, but doesn't link it to the package that requested it
• Putting this dependency into the devDependencies works as expected
• Adding it to both devDependencies and optionalDependencies produces the error ERROR: Internal Error: Cannot find shrinkwrap entry dependency ... for temp project: ...

[octogonz]

So i am hitting this issue but with latest pnpm. in my own package.json we have a optional dep that only for windows, but on windows it still does not install

@jcgertig Could you open a new issue with repro steps?

[octogonz]

* The `.npmrc` setting does not seem to work at all, the package is always downloaded, but not always linked correctly

We should document this, but generally we try to avoid putting settings in .npmrc for a few reasons:

• That file is interpreted inconsistently by different package managers, and its intended schema is not specified very rigorously
• As a result it's difficult to validate this file; Rush cannot easily catch mistakes such as a misspelled setting
• The .npmrc settings can be inherited from various locations outside the monorepo folder, which interferes with our goal of deterministic installations. For example it is very frustrating if a build can fail in a lab machine but does not repro locally, due to some hidden setting somewhere

my use case is putting one dependency in optionalDependencies and manipulating the .npmrc:optional to exclude it from the installation for the Docker builds.

@wasd171 Can you explain the idea behind this in more detail (maybe with an example)? I have not seen this practice before.

If you are simply trying to reduce the set of that dependencies deployed to Docker, you can use rush deploy for that.

[wasd171]

@octogonz
My use case is the following: we are using Cypress as a testing framework. In order to get auto-complete in VS Code the cypress package needs to be installed. Cypress binaries are quite heavy to download and this slows down the build stage in Docker. My idea was to put cypress into optionalDependencies so that the devs could have it on their computers but it would be excluded from Docker. Turns out that there is an env variable that allows to skip binary download for Cypress, so the issue was resolved, but overall I feel that there might be other cases where usage of the optionalDependencies will be justified

[octogonz]

My idea was to put cypress into optionalDependencies so that the devs could have it on their computers but it would be excluded from Docker.

@wasd171 My understanding of optionalDependencies is that the package manager will always try to install them, but will keep going if it fails. The main use case is for native packages that don't support certain OS's, but frankly this feature always seemed a bit dubious to me.

As far as rush deploy, normally it's meant to be a full build, and then Cyprus would be excluded from the deployment because it's listed in devDependencies.

But it sounds like you want a way for rush install to behave differently on CI machines, to avoid avoid installing certain dependencies that are only needed on developer machines. If so this is really a separate topic from rush deploy, and something that could possibly be solved using the "autoinstallers" feature, or maybe the "filtered installs" feature, or also installation variants could work. But we'd need a more specific use case, ideally in a new GitHub issue that isn't mixed up with optionalDependencies.

[wasd171]

@octogonz yes, sounds like a better use case for the installation variants. I was mostly confused by the .npmrc not working as I'd expect