-
Notifications
You must be signed in to change notification settings - Fork 654
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
feat(grpc-reflection): created new @grpc/reflection package #2613
Conversation
following on from grpc/proposal#397 - @murgatroid99 FYI |
There was a problem hiding this 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.
…sage encoding/decoding
There was a problem hiding this 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.
…eflection API itself
done in 8016d87. It's a bit awkward because the node 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 |
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 |
…on the reflection API itself" This reverts commit 8016d87.
ah that makes a lot more sense - done |
There was a problem hiding this 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.
It's live on npm: https://www.npmjs.com/package/@grpc/reflection |
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 SpecificationThe main implementation logic is taken from [email protected] with a few minor changes, namely:
proto-loader-gen-types
for type generation to avoid adding a dependency onbuf
mocha
andassert
to avoid adding a dependency onjest
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 clientsYou 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)