cliDependencies and npm dependency

[joscha]

protobuf.js version: 6.6.5

When using the CLI version of protobuf.js, some additional dependencies are installed (they are defined without versions in cliDependencies of the package.json. There are a few issues with this approach that I hope we can find a solution for:

• The project I am using is using yarn, meaning that wherever pbjs was installed into does not have npm available on CI.
• Locally there is npm available, but quite often the install fails:

  
  $ ./proto/gen.js.sh
  installing jsdoc@^3.4.2
  installing tmp@0.0.31
  installing espree@^3.1.3
  installing escodegen@^1.8.1
  module.js:472
  throw err;
  ^

Error: Cannot find module 'espree' at Function.Module._resolveFilename (module.js:470:15) at Function.Module._load (module.js:418:25) at Module.require (module.js:498:17) at require (internal/module.js:20:19) at Object. (/Users/joscha/Development/wala/wala-sodaqone_3/node_modules/protobufjs/cli/targets/static.js:7:18) at Module._compile (module.js:571:32) at Object.Module._extensions..js (module.js:580:10) at Module.load (module.js:488:32) at tryModuleLoad (module.js:447:12) at Function.Module._load (module.js:439:3) error Command failed with exit code 1.

* The dependencies that are installed on-the-fly are not part of any `yarn.lock` or `npm-shrinkwrap.json`, meaning that the installation can 1) not be cached easily 2) not be reproduced easily 3) yarn will blow away any installed packages not part of the lock file any time `yarn install` is run 4) `npm shrinkwrap` will wet itself because there are additional packages installed.

A couple solutions come to mind:
* Make these actual `dependencies` - after all they are used at runtime
* Create a `protobuf.js-cli` package that has a dependency on the `protobuf.js` package and on all the CLI dependencies. That way this package could be lightweight for the people that are not interested in the CLI.

What do you think?

[dcodeIO]

I introduced this because I noticed that a few people felt the need to fork protobuf.js without the cli to avoid the additional dependencies. The cleanest solution would be a -cli package probably.

[joscha]

I'd be happy to open a PR to make that happen - not sure how I would open a PR for the new CLI package though, do you want to create an empty repo for it in your name?

[dcodeIO]

I'd probably just include it here similar to the packages in lib/, making cli/ actually an npm package but excluding it from the top-level package through .npmignore. That should be fine because the cli package will most likely have to be tightly coupled to the main package anyway.

[tamird]

Just hit this in CockroachDB - one of these dependencies moved and broke our CI.

This is a seriously bad practice - @joscha are you still working on a solution?

[tamird]

Yep, looks like jsdoc 3.5.0 changed something which causes the way it is being called from https://github.com/dcodeIO/protobuf.js/blob/6.8.0/cli/pbts.js, which causes randomly truncated output.

My bet is that there's an error popping out somewhere and not being handled.

[joscha]

@tamird Not working on a solution, sorry, as far as I remember, there were a few commits made in that direction however.

[ahl]

We're hitting this too. Perhaps I'm doing something unusual, but the cli's use of npm install results in an npm_modules directory being adding in the protobufjs/cli subdirectory... a pretty surprising behavior.

@joscha the protobuf.js-cli package solution seems like a great idea; @dcodeIO it sounds like you're amenable to a PR for this?

[erickj]

@dcodeIO what are the tasks remaining to complete this? I'd be happy to work on this.

Currently I'm carrying around a few hacks in my build process to prevent the cli tool from installing npms at run time.

[couchand]

Ping. Is there anything happening here? Fixing this issue seems like it should be pretty high priority, since shadow invoking NPM is such a dark pattern.

[raytung]

hey @dcodeIO, thanks for creating protobuf.js. Is there anything we can do to help get this CLI package over the line?

[alejandroclaro]

Hi there!

any solution for this issue?

It seems to have been open for a long time. We are experiencing the same problem on a monorepo with yarn workspaces and lerna. Due to concurrency, sometimes installing those dependencies using npm during the cli invocation causes very bad things to happen.

As someone mentioned, I think this is a very bad practice. These hidden dependencies are highly suspicious to any auditor and are undoubtedly the source of many problems.

[cgeiershouse]

Hi there!

any solution for this issue?

It seems to have been open for a long time. We are experiencing the same problem on a monorepo with yarn workspaces and lerna. Due to concurrency, sometimes installing those dependencies using npm during the cli invocation causes very bad things to happen.

As someone mentioned, I think this is a very bad practice. These hidden dependencies are highly suspicious to any auditor and are undoubtedly the source of many problems.

On my team, we worked around by using https://github.com/ds300/patch-package to remove the calls to util.setup() in cli/pbjs.js and cli/pbts.js, and then made sure to list the relevant dependencies as devDependencies of our project.

[pauldraper]

Truly, finding npm install in protobuf.js was one of the biggest wtf moments of my life. Like summoning the dead to solve a minor staffing issue.

Related to #1368

Could be fixed by #1234

[dcodeIO]

But it was a big moment!

[insidewhy]

Our build broke because marked is not park of yarn.lock and now uses optional chaining, so won't work in node 10 anymore. Despite breaking backwards compatibility, this didn't happen with a major dependency version upgrade, and when pbts does its un-shrink-wrapped install, it just gets whatever latest version fulfills the dependency. :facepalm: