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 server support for Connect HTTP GET requests #624

Merged
merged 26 commits into from
May 12, 2023

Conversation

jchadwick-buf
Copy link
Contributor

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

Adds support for handling Connect HTTP GET requests in servers. We also expose protocolName and requestMethod on handler context so that handlers can know how the RPC request was initiated.

It's necessary to handle parsing and decoding URL query parameters
ourselves due to the fact that JavaScript/the DOM lack the functionality
needed to handle potentially binary data in URL-encoded form.

In the future, we may be able to improve performance by determining if
we need to do this using a regular expression.
@jchadwick-buf jchadwick-buf requested a review from timostamm May 8, 2023 20:50
Base automatically changed from jchadwick/unary-get-support to main May 10, 2023 17:03
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.

Hey John, just two code style comments from me. I believe this is ready to merge otherwise.
Some more tests would be nice to have, so I put up #629, based on this PR.

import { parseTimeout } from "./parse-timeout.js";
import { paramMessage, paramBase64 } from "./query-params.js";
Copy link
Member

Choose a reason for hiding this comment

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

Can you merge with import statement on line 73? The TypeScript language service can do that. Your IDE should have a command to organize imports.

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 ran organize imports. I didn't really realize it handled our import style correctly, because some other tsserver functions don't seem to handle it very well. Thanks for the tip.

packages/connect/src/protocol-connect/handler-factory.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.

LGTM, thank you for this!

Please add a PR description so our release changelog is comprehensible.

@jchadwick-buf
Copy link
Contributor Author

Thanks for your help + additional tests!

@jchadwick-buf jchadwick-buf merged commit fc9cd63 into main May 12, 2023
@jchadwick-buf jchadwick-buf deleted the jchadwick/unary-get-server branch May 12, 2023 16:36
@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.

3 participants