Skip to content
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

missing @grpc/grpc-js dependency #879

Closed
JimPatterson opened this issue Feb 5, 2020 · 6 comments
Closed

missing @grpc/grpc-js dependency #879

JimPatterson opened this issue Feb 5, 2020 · 6 comments
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@JimPatterson
Copy link

Environment details

  • OS: Alpine Linux 3.9
  • Node.js version: 12.13.0
  • npm version: pnpm 4.9.2 instead of npm
  • @google-cloud/pubsub version: 1.4.1

Error seen

When trying to require pubsub, I get an error that @grpc/grpc-js can't found like:

Thrown:
Error: Cannot find module '@grpc/grpc-js'
Require stack:
- /beacon/pnpm-store/node_modules/.pnpm/registry.npmjs.org/@google-cloud/pubsub/1.4.1/node_modules/@google-cloud/pubsub/build/src/pubsub.js
- /beacon/pnpm-store/node_modules/.pnpm/registry.npmjs.org/@google-cloud/pubsub/1.4.1/node_modules/@google-cloud/pubsub/build/src/index.js
- /beacon/pnpm-store/node_lib/storage_mdd/src/sendChangeTasks.js

The error is being seen due to the non-flat node_modules layout that pnpm uses where npm's flat style would likely not have seen the problem. But the real cause of the problem is due to pubsub having a require of grpc-js without declaring it as a dependency. I see older issue talking about this:

I'm not a Typescript user so I can't review the comment about only needing it for types, but https://github.com/googleapis/nodejs-pubsub/blob/master/src/publisher/publish-error.ts#L17 imports @grpc/grpc-js directly and the resultant javascript does also. This implies that there is a direct dependency on @grpc/grpc-js

@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Feb 5, 2020
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Feb 6, 2020
@feywind feywind added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Feb 8, 2020
@feywind
Copy link
Collaborator

feywind commented Feb 8, 2020

Thanks @JimPatterson for the report! There are some other issues going on regarding grpc-js, so I'll probably look at this in that context.

@yoshi-automation yoshi-automation added triage me I really want to be triaged. 🚨 This issue needs some love. labels Feb 8, 2020
@bcoe bcoe added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Feb 10, 2020
@bcoe
Copy link
Contributor

bcoe commented Feb 10, 2020

@feywind @JimPatterson 👋 google-gax which is depended on by @google-cloud/pubsub has @grpc/grpc-js as a dependency, so it's definitely in the dependency graph of a fresh installation.

This sounds like it's potentially an issue with pnpm, mind testing out with an npm install, and seeing if things do work for you as expected?

@yoshi-automation yoshi-automation removed triage me I really want to be triaged. 🚨 This issue needs some love. labels Feb 10, 2020
@feywind
Copy link
Collaborator

feywind commented Feb 10, 2020

@bcoe I guess my quick triage of the question last week... the Pub/Sub code is directly require()'ing grpc-js. e.g.:

index.ts:export {ServiceError} from '@grpc/grpc-js';

It seems like that's making some shaky assumptions about the availability of the grpc-js module to the pub/sub module. I'm guessing we decided to pull it in through gax to manage versions there, but it doesn't seem technically correct to me.

On the other hand @JimPatterson as Ben was saying to me internally, I think we really only support using npm, so yarn and pnpm are a bit of a best effort kind of thing. I'm unsure we'll be changing this for non-npm use.

@sonu27
Copy link

sonu27 commented Feb 20, 2020

I'm getting this issue too using npm on alpine.

Adding @grpc/grpc-js using npm i fixes the problem. But I use the grpc which is more complete and faster. I guess the pubsub code shouldn't reference the JS lib.

@feywind
Copy link
Collaborator

feywind commented May 5, 2020

I think that post-2.x I'm planning to just add @grpc/grpc-js as a dependency for pubsub. Pulling in the constants from that seems okay, even for gRPC C++ usage, but it shouldn't be assuming it'll be there in the tree, in my opinion.

Incidentally, there have been a lot of updates to grpc-js lately. I'm also hoping to get us on the 1.x version of that after the pubsub 2.0 is released. So perhaps that will let you move on to the pure JS version for your use case. I've been told to try to encourage that, because gRPC C++ for Node will be moving into maintenance mode soon (no new features).

@feywind
Copy link
Collaborator

feywind commented Aug 18, 2020

I forgot to update this - we talked it over and decided it's probably best if Pub/Sub itself doesn't gain a direct dependency on grpc-js, because the version could fall out of step with gax itself. And some typings there are needed in the Pub/Sub library. So I'm really not in love with this solution, but I think for now the plan is not to change this.

As a workaround for package manager issues outside stock npm, sonu27's comment above should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. priority: p2 Moderately-important priority. Fix may not be included in next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

5 participants