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

feat(grpc-reflection): created new @grpc/reflection package #2613

Merged
merged 13 commits into from
Dec 1, 2023

Conversation

jtimmons
Copy link
Contributor

Adding a new @grpc/reflection package as outlined in gRFC L108 which will allow users to add reflection capabilities to their server according to the gRPC Server Reflection API Specification

The main implementation logic is taken from nestjs-grpc-reflection@0.2.0 with a few minor changes, namely:

  1. Swapping to use proto-loader-gen-types for type generation to avoid adding a dependency on buf
  2. Changes to the test cases to use mocha and assert to avoid adding a dependency on jest

Testing

This PR includes a sample gRPC service API under proto/sample which makes use of a wide variety of protobuf features including mixed proto2/proto3 files, extensions, nested and shadowed messages, and other esoteric cases that are used both in the unit tests and also in live tests with developer clients

You can test the reflection capability by starting the server like so:

$ cd packages/grpc-reflection
$ npx ts-node example/server.ts

this starts the server on localhost:5000. You can then use a gRPC dev client such as grpcui to reflect the schema from the server and make requests to it (though there are no request handlers)

@jtimmons
Copy link
Contributor Author

following on from grpc/proposal#397 - @murgatroid99 FYI

Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the individual comments, the example code shouldn't be inside of the package. We have an existing directory for examples. We're also trying to unify the example content across different languages, so for this, the model would be the Go reflection example. You can make that, or I can in a separate PR, but either way the example code currently in this PR should be removed.

packages/grpc-reflection/package.json Outdated Show resolved Hide resolved
packages/grpc-reflection/package.json Outdated Show resolved Hide resolved
packages/grpc-reflection/package.json Outdated Show resolved Hide resolved
packages/grpc-reflection/package.json Outdated Show resolved Hide resolved
packages/grpc-reflection/package.json Outdated Show resolved Hide resolved
gulpfile.ts Outdated Show resolved Hide resolved
packages/grpc-reflection/README.md Outdated Show resolved Hide resolved
Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good, but in the existing example I linked, the server serves both the reflection service, and the services it is reflecting. I think the one we have here should do the same, because that is a better demonstration of what reflection is supposed to do.

@jtimmons
Copy link
Contributor Author

This mostly looks good, but in the existing example I linked, the server serves both the reflection service, and the services it is reflecting. I think the one we have here should do the same, because that is a better demonstration of what reflection is supposed to do.

done in 8016d87. It's a bit awkward because the node grpc.Server doesn't expose all of its loaded services publicly in the same way that the go server does so I had to pull the proto-file references from the package explicitly. Let me know if you think that muddies the example too much

this can also be improved a bit once we address the file indexing problem from #2595. If we don't need to worry about file name collisions during indexing then we can simply add the reflection API specs internally to the ReflectionService

@murgatroid99
Copy link
Member

That's the inverse of what I was talking about. My problem was not that the reflection data didn't contain the reflection service, but that the server wasn't actually serving the Greeter service. If we use a loaded proto file (helloworld.proto) to register a service on the server, and we use that same loaded proto file to initialize the reflection data, I think that would be a much clearer demonstration of how someone is intended to use this library in practice.

@jtimmons
Copy link
Contributor Author

That's the inverse of what I was talking about. My problem was not that the reflection data didn't contain the reflection service, but that the server wasn't actually serving the Greeter service. If we use a loaded proto file (helloworld.proto) to register a service on the server, and we use that same loaded proto file to initialize the reflection data, I think that would be a much clearer demonstration of how someone is intended to use this library in practice.

ah that makes a lot more sense - done

Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good for the initial release.

@murgatroid99 murgatroid99 merged commit d46360d into grpc:master Dec 1, 2023
9 of 10 checks passed
@murgatroid99
Copy link
Member

It's live on npm: https://www.npmjs.com/package/@grpc/reflection

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants