-
Notifications
You must be signed in to change notification settings - Fork 23
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
Implement gRPC-Web protocol #89
Conversation
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerRequestImpl.java
Outdated
Show resolved
Hide resolved
@vietj I've removed the milestone and label, they are already set on the corresponding issue |
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerRequestImpl.java
Outdated
Show resolved
Hide resolved
@vietj PTAL |
a6764db
to
7b87dd5
Compare
These tests have been built by inspection of a TCP traffic capture between a browser and the Envoy proxy, configured as recommended by the gRPC-Web project. More tests will be added in follow-up commits Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
Including about CORS setup in Vert.x Web. Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
https://github.com/grpc/grpc-web/blob/a639b4cf2611de2b68883571787083b73cf61f5e/doc/interop-test-descriptions.md Create gRPC-Web prereqs image from a main branch checkout: the prereqs image on Docker Hub might not always be up-to-date (confirmed by gRPC-Web maintainer). Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
@vietj I've squashed commits to ease the next review |
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.
A few comments to address
vertx-grpc-common/src/main/java/io/vertx/grpc/common/impl/GrpcMessageImpl.java
Show resolved
Hide resolved
vertx-grpc-common/src/main/java/io/vertx/grpc/common/GrpcMediaType.java
Outdated
Show resolved
Hide resolved
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerImpl.java
Outdated
Show resolved
Hide resolved
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerImpl.java
Show resolved
Hide resolved
vertx-grpc-server/src/main/java/io/vertx/grpc/server/GrpcServerOptions.java
Outdated
Show resolved
Hide resolved
vertx-grpc-common/src/main/java/io/vertx/grpc/common/impl/GrpcMessageImpl.java
Outdated
Show resolved
Hide resolved
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerResponseImpl.java
Outdated
Show resolved
Hide resolved
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerResponseImpl.java
Outdated
Show resolved
Hide resolved
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerRequestImpl.java
Outdated
Show resolved
Hide resolved
@vietj PTAL |
Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerImpl.java
Show resolved
Hide resolved
The browser does not expose HTTP protocol details so, even if the client and server communicate over HTTP/2, standard gRPC cannot be used and gRPC-Web is mandatory. Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
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.
A few minor things and we are good to go
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerResponseImpl.java
Outdated
Show resolved
Hide resolved
vertx-grpc-server/src/main/java/io/vertx/grpc/server/impl/GrpcServerResponseImpl.java
Outdated
Show resolved
Hide resolved
vertx-grpc-common/src/main/java/io/vertx/grpc/common/impl/GrpcMessageImpl.java
Show resolved
Hide resolved
Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
@vietj PTAL |
Closes #55
Each commit can be reviewed individually.