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

fix(Grpc-Web): Fix compilation failure when a service definition contains a client streaming call. #535

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion integration/grpc-web/example-test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { DashStateClientImpl } from './example';
import { EMPTY } from 'rxjs';

describe('grpc-web', () => {
it('at least compiles', () => {
Expand All @@ -19,5 +20,14 @@ describe('grpc-web', () => {
const client = new DashStateClientImpl(rpc);
const userSettings = client.UserSettings;
userSettings({});
})
});
it('throws on client streaming call', () => {
const rpc = {
unary: jest.fn(),
invoke: jest.fn(),
};
const client = new DashStateClientImpl(rpc);
const call = () => client.ChangeUserSettingsStream(EMPTY)
expect(call).toThrowError("ts-proto does not yet support client streaming!")
});
});
Binary file modified integration/grpc-web/example.bin
Binary file not shown.
2 changes: 2 additions & 0 deletions integration/grpc-web/example.proto
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package rpx;
service DashState {
rpc UserSettings(Empty) returns (DashUserSettingsState);
rpc ActiveUserSettingsStream(Empty) returns (stream DashUserSettingsState);
// not supported in grpc-web, but should still compile
rpc ChangeUserSettingsStream (stream DashUserSettingsState) returns (stream DashUserSettingsState) {}
}

message DashFlash {
Expand Down
13 changes: 13 additions & 0 deletions integration/grpc-web/example.ts
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,11 @@ export const Empty = {
export interface DashState {
UserSettings(request: DeepPartial<Empty>, metadata?: grpc.Metadata): Promise<DashUserSettingsState>;
ActiveUserSettingsStream(request: DeepPartial<Empty>, metadata?: grpc.Metadata): Observable<DashUserSettingsState>;
/** not supported in grpc-web, but should still compile */
ChangeUserSettingsStream(
request: Observable<DeepPartial<DashUserSettingsState>>,
metadata?: grpc.Metadata
): Observable<DashUserSettingsState>;
}

export class DashStateClientImpl implements DashState {
Expand All @@ -604,6 +609,7 @@ export class DashStateClientImpl implements DashState {
this.rpc = rpc;
this.UserSettings = this.UserSettings.bind(this);
this.ActiveUserSettingsStream = this.ActiveUserSettingsStream.bind(this);
this.ChangeUserSettingsStream = this.ChangeUserSettingsStream.bind(this);
}

UserSettings(request: DeepPartial<Empty>, metadata?: grpc.Metadata): Promise<DashUserSettingsState> {
Expand All @@ -613,6 +619,13 @@ export class DashStateClientImpl implements DashState {
ActiveUserSettingsStream(request: DeepPartial<Empty>, metadata?: grpc.Metadata): Observable<DashUserSettingsState> {
return this.rpc.invoke(DashStateActiveUserSettingsStreamDesc, Empty.fromPartial(request), metadata);
}

ChangeUserSettingsStream(
request: Observable<DeepPartial<DashUserSettingsState>>,
metadata?: grpc.Metadata
): Observable<DashUserSettingsState> {
throw new Error('ts-proto does not yet support client streaming!');
}
}

export const DashStateDesc = {
Expand Down
27 changes: 19 additions & 8 deletions src/generate-grpc-web.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { MethodDescriptorProto, FileDescriptorProto, ServiceDescriptorProto } from 'ts-proto-descriptors';
import { requestType, responsePromiseOrObservable, responseType } from './types';
import { rawRequestType, requestType, responsePromiseOrObservable, responseType } from './types';
import { Code, code, imp, joinCode } from 'ts-poet';
import { Context } from './context';
import { assertInstanceOf, FormattedMethodDescriptor, maybePrefixPackage } from './utils';
Expand Down Expand Up @@ -35,33 +35,44 @@ export function generateGrpcClientImpl(
assertInstanceOf(methodDesc, FormattedMethodDescriptor);
chunks.push(code`this.${methodDesc.formattedName} = this.${methodDesc.formattedName}.bind(this);`);
}
chunks.push(code`}\n`);
chunks.push(code`}`);

// Create a method for each FooService method
for (const methodDesc of serviceDesc.method) {
chunks.push(generateRpcMethod(ctx, serviceDesc, methodDesc));
}

chunks.push(code`}`);
return joinCode(chunks, { trim: false });
return joinCode(chunks, { trim: false, on: '\n' });
}

/** Creates the RPC methods that client code actually calls. */
function generateRpcMethod(ctx: Context, serviceDesc: ServiceDescriptorProto, methodDesc: MethodDescriptorProto) {
assertInstanceOf(methodDesc, FormattedMethodDescriptor);
const { options, utils } = ctx;
const inputType = requestType(ctx, methodDesc);
const partialInputType = code`${utils.DeepPartial}<${inputType}>`;
const requestMessage = rawRequestType(ctx, methodDesc);
const inputType = requestType(ctx, methodDesc, true);
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of invoking both rawRequestType + requestType, I wonder if requestType would return a tuple or something...

Just thinking out loud, b/c we also have an open issue where someone's input type is not actually a message, so we need to make the fromPartial call optional, and maybe that'd be nice to handle inside of requestType itself... 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I think there is a bit of a code smell that the requestType function manages the wrapping of the type into partial, but it was a minimal change. I suspect it might need a bit of a rethink for that other issue

const returns = responsePromiseOrObservable(ctx, methodDesc);

if (methodDesc.clientStreaming) {
return code`
${methodDesc.formattedName}(
request: ${inputType},
metadata?: grpc.Metadata,
): ${returns} {
throw new Error('ts-proto does not yet support client streaming!');
}
`;
}

const method = methodDesc.serverStreaming ? 'invoke' : 'unary';
return code`
${methodDesc.formattedName}(
request: ${partialInputType},
request: ${inputType},
metadata?: grpc.Metadata,
): ${returns} {
return this.rpc.${method}(
${methodDescName(serviceDesc, methodDesc)},
${inputType}.fromPartial(request),
${requestMessage}.fromPartial(request),
metadata,
);
}
Expand Down
6 changes: 2 additions & 4 deletions src/generate-services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,10 @@ export function generateService(
params.push(code`ctx: Context`);
}

let inputType = requestType(ctx, methodDesc);
// the grpc-web clients auto-`fromPartial` the input before handing off to grpc-web's
// serde runtime, so it's okay to accept partial results from the client
if (options.outputClientImpl === 'grpc-web') {
inputType = code`${utils.DeepPartial}<${inputType}>`;
}
const partialInput = options.outputClientImpl === 'grpc-web';
const inputType = requestType(ctx, methodDesc, partialInput);
Copy link
Owner

Choose a reason for hiding this comment

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

Cool, nice clean up!

params.push(code`request: ${inputType}`);

// Use metadata as last argument for interface only configuration
Expand Down
4 changes: 3 additions & 1 deletion src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,9 @@ export function generateFile(ctx: Context, fileDesc: FileDescriptorProto): [stri
chunks.push(generateGrpcClientImpl(ctx, fileDesc, serviceDesc));
chunks.push(generateGrpcServiceDesc(fileDesc, serviceDesc));
serviceDesc.method.forEach((method) => {
chunks.push(generateGrpcMethodDesc(ctx, serviceDesc, method));
if (!method.clientStreaming) {
chunks.push(generateGrpcMethodDesc(ctx, serviceDesc, method));
}
if (method.serverStreaming) {
hasServerStreamingMethods = true;
}
Expand Down
7 changes: 6 additions & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -652,8 +652,13 @@ export function rawRequestType(ctx: Context, methodDesc: MethodDescriptorProto):
return messageToTypeName(ctx, methodDesc.inputType);
}

export function requestType(ctx: Context, methodDesc: MethodDescriptorProto): Code {
export function requestType(ctx: Context, methodDesc: MethodDescriptorProto, partial: boolean = false): Code {
let typeName = rawRequestType(ctx, methodDesc);

if (partial) {
typeName = code`${ctx.utils.DeepPartial}<${typeName}>`;
}

if (methodDesc.clientStreaming) {
return code`${imp('Observable@rxjs')}<${typeName}>`;
}
Expand Down