-
Notifications
You must be signed in to change notification settings - Fork 649
[TS] Http procedure API #3731
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
[TS] Http procedure API #3731
Conversation
acd46ea to
6e2ece6
Compare
7495076 to
41669b0
Compare
gefjon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel comfortable enough with either our V8 internals or TypeScript bindings to do an in-depth review, but I like the API design here. I appreciate the cross-language consistency of keeping HTTP-related methods in ctx.http, and also the similarity between your fetch method and the browser env's fetch function.
Centril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks mostly good. I'd like to request some changes below though before merging.
Description of Changes
Provides a fetch-alike API on
ctx.http. I guess it could just bectx.fetch()instead ofctx.http.fetch(), but I'm not sure if that's a good idea.Expected complexity level and risk
2
Testing