request: remove grpc dependency from @grpc/health-check
cjihrig opened this issue · 8 comments
Is your feature request related to a problem? Please describe.
Not necessarily a problem, but I've had a request to fork this package. I don't really want to. From what I can tell, grpc.makeGenericClientConstructor() and grpc.status.NOT_FOUND are the only production dependencies on the grpc module.
Describe the solution you'd like
If health.js exported a function that accepted a grpc-like module, the module could use that as its gRPC dependency.
Describe alternatives you've considered
Forking - but that seems a bit extreme for such a small module.
I agree with changing this, it has just been low priority. I believe the proper solution here is for the package to export a PackageDefinition object, plus a service implementation object. This just leaves the issue of grpc.status.NOT_FOUND. My preferred solution for that is to create an @grpc/status package, or maybe @grpc/constants, and define status there so that the health checker package can depend on it without pulling in a whole grpc implementation. An alternative or temporary solution would be to depend on @grpc/grpc-js, which is pretty thin, and only use it for the status constants.
Any updates on when this could be tackled? I am also currently looking into refactor my health checks to use this package.
We are currently using @grpc/grpc-js and it would be a pretty big inconvenience if we would need to pull in grpc as a dependency.
I could also make a PR with migrating to @grpc/grpc-js as this would be the only thing that's achievable from a contribution perspective since the community has no way of creating @grpc/constants or @grpc/status.
Sorry, this has still been low priority for us. I would accept a PR, but it's about more than just switching the dependency to @grpc/grpc-js. That would just shift the problem around, so that the library no longer works with grpc. The important thing is to have it export a generic PackageDefinition object that can be loaded into either library, instead of the Client object generated by grpc.makeGenericClientConstructor(). It would still need to depend on @grpc/grpc-js, to get access to status.NOT_FOUND, but it shouldn't export anything from that library.
In addition, I think it would probably be a good idea to switch to @grpc/proto-loader for handling the protobuf types. I'm not comfortable with the way the current modules pollute the global namespace, and it's probably easier to use and maintain than the current protoc + google-protobuf implementation. That could be done in a separate change, but some change needs to be made to the current protobuf handling to decouple it from grpc.
Has there been any progress on this or at the very least moving the dependency to grpc-js? With the upcoming EOL of grpc, it would be nice to be able to fully transition to grpc-js.
@murgatroid99 if the grpc module is being EOL'd, what is the need to keep backwards compatibility? Could a major version be pushed to npm as @grpc/health-check-js?
For others running into issues on node 15 with the grpc build, I published the simpler patch to npm as @bermilabs/grpc-health-check@2.0.0
@bermi Looks like the changes required are pretty minimal... might be worth creating a PR back to this project.
Until this package is upgraded, grpc-js-health-check is a drop-in pure js version of grpc-health-check
I just created a proposal for a new API for this library that would include this change, at grpc/proposal#391. Please comment there if you have any feedback on that design.