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

feat: JS clients can provide custom grpc transports #6476

Merged
merged 15 commits into from
Dec 10, 2024

Conversation

niloc132
Copy link
Member

Provides a contract for client applications to use a custom http/2 implementation. Roughly abstracted from our TypeScript grpc-web library, with a few rough edges taken off, and no external dependencies.

Two integration tests are included, one which requires https (presently only possible with manual testing, see #6421), and one which pretends to contact the server but really responds to every request with a "success" response and no payload.

No documentation required at this time, generated typescript includes details on the new APIs.

Fixes #6404

bmingles
bmingles previously approved these changes Dec 10, 2024
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Things look good to me. Verified I can connect from extension.

@niloc132 niloc132 requested a review from bmingles December 10, 2024 21:07
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

re-approving after test fix

@niloc132 niloc132 merged commit f5adda6 into deephaven:main Dec 10, 2024
17 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2024
Comment on lines 68 to 70
// if (map.has("fetch")) {
// fetch = map.getAsAny("fetch").uncheckedCast();
// }
Copy link
Member

Choose a reason for hiding this comment

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

This comment could be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be, though we thought we would want to have it - in theory this is a simpler version of what we just implemented.

If we do decide we'll never want it, I think I'd want to expose canonical factories for fetch and websocket, so users can just assign them if they want to direct which built-in factory to use.

}
return useWebsockets;
public boolean supportsClientStreaming() {
return getOptions().transportFactory.getSupportsClientStreaming();
Copy link
Member

Choose a reason for hiding this comment

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

While it's correct since the transportFactory is always initialized correctly by IdeConnection, in ConnectionOptions it annotates transportFactory as JsNullable. I guess we'd just throw here if it was null anyways?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - the annotations are for the sake of the generated TS and docs ("you can populate this or not"). In theory the compiler could take them seriously, but in practice Java isn't expressive enough about nulls, even with this annotation (static analysis is used instead, to solve for fields/variables that can never be null).

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

Successfully merging this pull request may close these issues.

Allow consumer of jsapi to provide a custom gRPC transport
3 participants