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

Add service/method name to RpcError #197

Merged

Conversation

jcready
Copy link
Contributor

@jcready jcready commented Dec 31, 2021

Fixes #195

Copy link
Owner

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Thank you for looking into all cases! I only have two nits.

Comment on lines 73 to 79
clientStreaming<I extends object, O extends object>(method?: MethodInfo<I, O>/*, options: RpcOptions*/): ClientStreamingCall<I, O> {
const e = new RpcError('Client streaming is not supported by grpc-web', GrpcStatusCode[GrpcStatusCode.UNIMPLEMENTED]);
if (method) {
e.methodName = method.name;
e.serviceName = method.service.typeName;
}
throw e;
Copy link
Owner

Choose a reason for hiding this comment

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

We are safe to make the argument required. interface RpcTransport requires both arguments - I think I just commented them out because my IDE complained about them being unused.

Suggested change
clientStreaming<I extends object, O extends object>(method?: MethodInfo<I, O>/*, options: RpcOptions*/): ClientStreamingCall<I, O> {
const e = new RpcError('Client streaming is not supported by grpc-web', GrpcStatusCode[GrpcStatusCode.UNIMPLEMENTED]);
if (method) {
e.methodName = method.name;
e.serviceName = method.service.typeName;
}
throw e;
clientStreaming<I extends object, O extends object>(method: MethodInfo<I, O>/*, options: RpcOptions*/): ClientStreamingCall<I, O> {
const e = new RpcError('Client streaming is not supported by grpc-web', GrpcStatusCode[GrpcStatusCode.UNIMPLEMENTED]);
e.methodName = method.name;
e.serviceName = method.service.typeName;
throw e;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this optional so that the previous unit tests would pass (it didn't pass in the MethodInfo). Making this optional was a smaller change, but I will make this required and update the unit test.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I was unaware. Thanks for the update!

Comment on lines 82 to 88
duplex<I extends object, O extends object>(method?: MethodInfo<I, O>/*, options: RpcOptions*/): DuplexStreamingCall<I, O> {
const e = new RpcError('Duplex streaming is not supported by grpc-web', GrpcStatusCode[GrpcStatusCode.UNIMPLEMENTED]);
if (method) {
e.methodName = method.name;
e.serviceName = method.service.typeName;
}
throw e;
Copy link
Owner

Choose a reason for hiding this comment

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

Same here - safe to require the argument.

Suggested change
duplex<I extends object, O extends object>(method?: MethodInfo<I, O>/*, options: RpcOptions*/): DuplexStreamingCall<I, O> {
const e = new RpcError('Duplex streaming is not supported by grpc-web', GrpcStatusCode[GrpcStatusCode.UNIMPLEMENTED]);
if (method) {
e.methodName = method.name;
e.serviceName = method.service.typeName;
}
throw e;
duplex<I extends object, O extends object>(method: MethodInfo<I, O>/*, options: RpcOptions*/): DuplexStreamingCall<I, O> {
const e = new RpcError('Duplex streaming is not supported by grpc-web', GrpcStatusCode[GrpcStatusCode.UNIMPLEMENTED]);
e.methodName = method.name;
e.serviceName = method.service.typeName;
throw e;

@jcready jcready requested a review from timostamm January 4, 2022 16:03
@timostamm timostamm merged commit bc3aaf9 into timostamm:master Jan 4, 2022
@jcready jcready deleted the rpc-error-service-and-method-names branch January 7, 2022 16:13
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.

Add service/method name to RpcError
2 participants