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

pbjs, pbts dependencies #618

Closed
mkmik opened this Issue Jan 4, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@mkmik
Contributor

mkmik commented Jan 4, 2017

protobuf.js version: 6.4.0

run pbjs and not have it lazily load dependencies

$ ./node_modules/.bin/pbjs
installing minimist@^1.2.0 ...
module.js:472
    throw err;
    ^

Error: Cannot find module 'minimist'
$ ./node_modules/.bin/pbjs
installing chalk@^1.1.3 ...
installing glob@^7.1.1 ...
installing uglify-js@^2.7.5 ...
protobuf.js v6.4.1 CLI for JavaScript

Consolidates imports and converts between file formats.
...

i.e. it takes two runs of the pbjs to download the required dependencies in node_modules/protobufjs/node_modules, same goes for pbts (which blocks on Error: Cannot find module 'jsdoc/package.json')

The problem is caused by the fact that the dependencies required for the provided binaries are declared as devDependencies, but If I understand correctly devDependencies are meant to be dependencies needed to develop a given package, i.e. stuff that is only useful while developing protobufjs itself.

However, pbjs and pbts are tools that are meant to be used as devDependencies in projects that are using protobufjs.

I see that protobufjs has a small dependency set, and that's a good thing; thus we cannot just move

Would it make sense to put these scripts in a separate package, exposing its dependencies as "dependencies", so that other packages can depend on the scripts with "devDependencies"?

I'm I missing something?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 4, 2017

I did this because I noticed that some people forked protobuf.js v5 in order to remove the pbjs dependencies from the package. So I thought that making them install on demand once by looking them up from devDependencies would be a good thing.

In my tests, this mechanism worked fine. Must have missed something there.

dcodeIO added a commit that referenced this issue Jan 4, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 4, 2017

This should now install only production dependencies (which are usually already installed) plus the ones required by the respective CLI all at once (just before requiring them).

@ehallander9591

This comment has been minimized.

ehallander9591 commented Jan 4, 2017

I have a feeling this is why the glob isn't seen any more when I run pbjs

dcodeIO added a commit that referenced this issue Jan 5, 2017

@ghost ghost referenced this issue Jan 6, 2017

Closed

pbts cannot find jsdoc #622

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 6, 2017

This should finally be working now with 6.4.4. Give it a try and feel free to reopen if necessary.

@dcodeIO dcodeIO closed this Jan 6, 2017

@kirbysayshi

This comment has been minimized.

kirbysayshi commented Jan 31, 2017

I was just bitten by this bug when including protobufjs within a lerna-powered project. A package had pbjs as a prepublish step, which of course failed the first time. It's bizarre to run the exact same command a second time and have it work. In addition, this behavior assumes npm is available, which might not be, and could even wreck havoc when using something like yarn (it's unlikely, but it's not like node_modules has a spec all package managers adhere to).

If the dependencies of pbjs are too heavy then perhaps pbjs should be a separate package? I'm not sure the dependency weight is actually a concern here compared to breaking developer assumptions.

@kirbysayshi

This comment has been minimized.

kirbysayshi commented Jan 31, 2017

Also as far as I can tell 6.4.4 doesn't exist. Was it a typo above, or not published?

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Jan 31, 2017

Also as far as I can tell 6.4.4 doesn't exist. Was it a typo above, or not published?

Latest released is 6.6.3, last 6.4.X was 6.4.6 (see).

this behavior assumes npm is available, which might not be, and could even wreck havoc when using something like yarn (it's unlikely, but it's not like node_modules has a spec all package managers adhere to).

Actually, cli utilities assume that node is installed, which uses node_modules and also includes npm.

If the dependencies of pbjs are too heavy then perhaps pbjs should be a separate package? I'm not sure the dependency weight is actually a concern here compared to breaking developer assumptions.

The current setup is there because both library and cli are tightly coupled, and because the npm package contains all the common things anyway. Dependency weight hasn't been an issue to me in the first place, I just noticed that others felt the need to fork protobuf.js without the cli.

Hence, it's merely a compromise (no additional package to manage, no extra work for me, yet a super small dependency footprint), but it should be working properly as of today. If it isn't with latest, let me know, and I'll reconsider splitting it up.

@HLFrye

This comment has been minimized.

HLFrye commented Mar 1, 2017

We ran into this issue today. We were initially running on version 6.4.5, but reproduced the issue on both 6.4.6 and 6.6.4.

Our current solution is to add a postinstall script to run pbjs and throw away the output, so that future invocations will work, but we'd prefer to get rid of this additional step if we can.

We invoke pbjs with the following command as part of our compilation script:
"node_modules/.bin/pbjs -t static-module ./path/to/*.proto > ./.compiledOutput/path/to/Api.js"

Running on OS X 10.11.6, node 7.1.0, npm 3.10.9, bash 3.2.57

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 1, 2017

What's the output of the first call? Any error messages?

@HLFrye

This comment has been minimized.

HLFrye commented Mar 2, 2017

Here's the output of the first and second call:
[hf@macbook ~/Dev/project (bugfix/AM2-191-unable-to-build-from-latest-develop)]$ node_modules/.bin/pbjs
installing glob@^7.1.1
installing jsdoc@^3.4.2
installing minimist@^1.2.0
installing tmp@0.0.31
installing espree@^3.1.3
installing escodegen@^1.8.1
installing estraverse@^4.2.0
module.js:474
throw err;
^

Error: Cannot find module 'espree'
at Function.Module._resolveFilename (module.js:472:15)
at Function.Module._load (module.js:420:25)
at Module.require (module.js:500:17)
at require (internal/module.js:20:19)
at Object. (/Users/hf/Dev/project/node_modules/protobufjs/cli/targets/static.js:7:18)
at Module._compile (module.js:573:32)
at Object.Module._extensions..js (module.js:582:10)
at Module.load (module.js:490:32)
at tryModuleLoad (module.js:449:12)
at Function.Module._load (module.js:441:3)
[hf@macbook ~/Dev/project (bugfix/AM2-191-unable-to-build-from-latest-develop)]$ node_modules/.bin/pbjs
protobuf.js v6.6.4 CLI for JavaScript

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Mar 2, 2017

7a94453#diff-5c910a1602407f405a311849c9096888

That should make it work with master.

@HLFrye

This comment has been minimized.

HLFrye commented Mar 6, 2017

Tested this today, and it solves the problem I was having. Thank you!

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