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

Conversation

zakhenry
Copy link
Contributor

Note this does not implement client streaming, instead the function will throw due to the feature not being implemented. This is a stop-gap until true client streaming support is added.

This PR relates to #329 (the original issue), and partially overlaps with #335 (a WIP PR that actually implements client streaming, but has stalled)

@zakhenry zakhenry force-pushed the fix/grpc-web-streaming-compilation-failure branch from 3d961fc to 16ad075 Compare March 15, 2022 20:46
Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

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

@zakhenry thanks for the PR! I agree this is a good baby step / mitigation until we have full support. I just had one comment about ts-node/ts-proto typo, but otherwise looks great! Ping when that's updated and I'll hit merge.

Thanks!

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!

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

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

Choose a reason for hiding this comment

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

Nit ts-proto :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh whoops! fixed.

@zakhenry zakhenry force-pushed the fix/grpc-web-streaming-compilation-failure branch from 16ad075 to 9313f19 Compare March 27, 2022 05:18
…ains a client streaming call.

Note this does not implement client streaming, instead the function will throw due to the feature not being implemented. This is a stop-gap until true client streaming support is added.
@zakhenry zakhenry force-pushed the fix/grpc-web-streaming-compilation-failure branch from 9313f19 to 7e6bf69 Compare March 27, 2022 19:00
@stephenh
Copy link
Owner

Thanks @zakhenry !

@stephenh stephenh merged commit 0c83892 into stephenh:main Mar 27, 2022
stephenh pushed a commit that referenced this pull request Mar 27, 2022
## [1.110.2](v1.110.1...v1.110.2) (2022-03-27)

### Bug Fixes

* **Grpc-Web:** Fix compilation failure when a service definition contains a client streaming call. ([#535](#535)) ([0c83892](0c83892))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.110.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants