Skip to content

std/http: make req.body a Reader#3575

Merged
ry merged 2 commits into
denoland:masterfrom
kevinkassimo:std/http/go_read_req
Jan 2, 2020
Merged

std/http: make req.body a Reader#3575
ry merged 2 commits into
denoland:masterfrom
kevinkassimo:std/http/go_read_req

Conversation

@kevinkassimo

@kevinkassimo kevinkassimo commented Jan 2, 2020

Copy link
Copy Markdown
Contributor

Follow-up and partially based on #3553

req.body is now a Reader. This should now be closer to the behavior of Go.

req.bodyStream() is also tweaked and no longer exposed to public.

Also add req.contentLength. It is also a field exposed in Go's Request struct.

@ry ry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cool - LGTM

@ry
ry merged commit 5cf2eb7 into denoland:master Jan 2, 2020
@sholladay

Copy link
Copy Markdown

I like that the API surface has been reduced, but using it is more convoluted now and there are many new ways to screw it up. It is not at all obvious what the buffer length should be and having to create it manually is annoying. It's a lot more lines (comparatively). We have to use a while loop instead of the lovely for-of, which means that a small typo can lead to an infinite loop.

I am skeptical of the "do everything like Go" philosophy. The consistency is nice and Go's APIs are generally nice. But this is not Go. We should use the best facilities available to us that express the intent. Doesn't an async generator fit the bill better? At least it's what most people would prefer to interact with, I think.

@hayd

hayd commented Jan 5, 2020

Copy link
Copy Markdown
Contributor

It'd be great to have a helper in std to make using this more concise.

caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
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.

4 participants