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 client support for Connect HTTP GET requests #620

Merged
merged 10 commits into from
May 10, 2023

Conversation

jchadwick-buf
Copy link
Contributor

@jchadwick-buf jchadwick-buf commented May 5, 2023

Adds support for Connect HTTP GET requests on the client, both in Node.JS and the browser. Requires specifying the transport option useHttpGet for now to enable support.

Server implementation: #624

@jchadwick-buf jchadwick-buf requested a review from timostamm May 5, 2023 18:27
@jchadwick-buf jchadwick-buf force-pushed the jchadwick/unary-get-support branch from aedf5fa to bcc2ea7 Compare May 5, 2023 18:27
@jchadwick-buf jchadwick-buf changed the title Add client support for Connect HTTP GET requests. Add client support for Connect HTTP GET requests May 5, 2023
/**
* @private Internal code, does not follow semantic versioning.
*/
export function transformConnectPostToGetRequest<
Copy link
Member

Choose a reason for hiding this comment

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

Great use of UnaryRequest 🙂

packages/connect/src/protocol-connect/get-request.ts Outdated Show resolved Hide resolved
Comment on lines +30 to +31
// TODO(jchadwick-buf): Three regex replaces seems excessive.
// Can we make protoBase64.enc more flexible?
Copy link
Member

Choose a reason for hiding this comment

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

protobuf-es uses this for JSON, and it doesn't seem ideal to add complexity, bundle size there for a feature that is not used by protobuf-es. I agree this isn't pretty, but I am not sure we have a really good alternative.

packages/connect/src/protocol/transport-options.ts Outdated Show resolved Hide resolved
Copy link
Member

@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.

We'll need a PR description. Aside from the comment nit below, and #620 (comment), this looks ready to merge to me.

packages/connect-web/src/connect-transport.ts Outdated Show resolved Hide resolved
@jchadwick-buf jchadwick-buf merged commit 7c9ffe9 into main May 10, 2023
@jchadwick-buf jchadwick-buf deleted the jchadwick/unary-get-support branch May 10, 2023 17:03
@timostamm timostamm mentioned this pull request May 17, 2023
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.

2 participants