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

cliDependencies and npm dependency #716

Open
joscha opened this Issue Mar 23, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@joscha

joscha commented Mar 23, 2017

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.<anonymous> (/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

This comment has been minimized.

Owner

dcodeIO commented Mar 23, 2017

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.

@dcodeIO dcodeIO added the enhancement label Mar 23, 2017

@joscha

This comment has been minimized.

joscha commented Mar 24, 2017

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

This comment has been minimized.

Owner

dcodeIO commented Mar 24, 2017

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.

dcodeIO added a commit that referenced this issue Mar 24, 2017

dcodeIO added a commit that referenced this issue Mar 27, 2017

@tamird

This comment has been minimized.

tamird commented Jul 10, 2017

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

This comment has been minimized.

tamird commented Jul 10, 2017

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.

tamird added a commit to tamird/cockroach that referenced this issue Jul 10, 2017

build: do not generate jsdoc comments in typings
Through a multitude of issues in protobufjs, we are now experiencing
an issue where `pbts` randomly produces truncated partial output; the
primary culprit is dcodeIO/protobuf.js#716 -
protobufjs' CLI tools are overly clever in trying to minimize their
dependency surface, and do so by running `npm install` directly when
invoked (!!!). Somewhat predictably, since these packages are not
pinned by yarn in the way that all other packages are, we are now
seeing their versions float, leading to the proximate cause, which is
jsdoc moving to 3.5.0. For an undiagnosed reason, jsdoc 3.5.0 doesn't
cooperate with the way it is called from `pbts` (which is by shelling
out (!)).

For now, disabling jsdoc is fine, and fixes the problem.

tamird added a commit to tamird/cockroach that referenced this issue Jul 10, 2017

build: hardcode a known-good version of jsdoc
Through a multitude of issues in protobufjs, we are now experiencing
an issue where `pbts` randomly produces truncated partial output; the
primary culprit is dcodeIO/protobuf.js#716 -
protobufjs' CLI tools are overly clever in trying to minimize their
dependency surface, and do so by running `npm install` directly when
invoked (!!!). Somewhat predictably, since these packages are not
pinned by yarn in the way that all other packages are, we are now
seeing their versions float, leading to the proximate cause, which is
jsdoc moving to 3.5.0. For an undiagnosed reason, jsdoc 3.5.0 doesn't
cooperate with the way it is called from `pbts` (which is by shelling
out (!)).
@joscha

This comment has been minimized.

joscha commented Jul 10, 2017

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

tamird added a commit to tamird/cockroach that referenced this issue Jul 11, 2017

build: hardcode a known-good version of jsdoc
Through a multitude of issues in protobufjs, we are now experiencing
an issue where `pbts` randomly produces truncated partial output; the
primary culprit is dcodeIO/protobuf.js#716 -
protobufjs' CLI tools are overly clever in trying to minimize their
dependency surface, and do so by running `npm install` directly when
invoked (!!!). Somewhat predictably, since these packages are not
pinned by yarn in the way that all other packages are, we are now
seeing their versions float, leading to the proximate cause, which is
jsdoc moving to 3.5.0. For an undiagnosed reason, jsdoc 3.5.0 doesn't
cooperate with the way it is called from `pbts` (which is by shelling
out (!)).
@ahl

This comment has been minimized.

ahl commented Oct 31, 2017

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

This comment has been minimized.

erickj commented Mar 8, 2018

@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

This comment has been minimized.

couchand commented Jul 9, 2018

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

This comment has been minimized.

raytung commented Sep 12, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment