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: use internal body if available for returning the response in its original form as much as possible #145

Merged
merged 4 commits into from
Feb 17, 2024

Conversation

usualoma
Copy link
Member

Fixes #144

Reuse the following implementation idea to get the body inside the nodejs Response object and use it to return a response if available.

node-server/src/utils.ts

Lines 13 to 24 in a8117ce

export function getResponseInternalBody(response: Response) {
if (!kState || !response || !response.body) return
const state = (response as any)[kState]
if (!state || !state.body) return
return state.body as {
source: string | Uint8Array | FormData | Blob | null
stream: ReadableStream
length: number | null
}
}

@usualoma
Copy link
Member Author

@yusukebe Would you please review?

@usualoma
Copy link
Member Author

By the way, the following code is still used.

/**
* If content-encoding is set, we assume that the response should be not decoded.
* Else if transfer-encoding is set, we assume that the response should be streamed.
* Else if content-length is set, we assume that the response content has been taken care of.
* Else if x-accel-buffering is set to no, we assume that the response should be streamed.
* Else if content-type is not application/json nor text/* but can be text/event-stream,
* we assume that the response should be streamed.
*/
const {
'transfer-encoding': transferEncoding,
'content-encoding': contentEncoding,
'content-length': contentLength,
'x-accel-buffering': accelBuffering,
'content-type': contentType,
} = resHeaderRecord
if (
transferEncoding ||
contentEncoding ||
contentLength ||
// nginx buffering variant
(accelBuffering && regBuffer.test(accelBuffering as string)) ||
!regContentType.test(contentType as string)
) {
outgoing.writeHead(res.status, resHeaderRecord)
await writeFromReadableStream(res.body, outgoing)
} else {
const buffer = await res.arrayBuffer()
resHeaderRecord['content-length'] = buffer.byteLength
outgoing.writeHead(res.status, resHeaderRecord)
outgoing.end(new Uint8Array(buffer))
}

Removing this will cause the following tests to fail

it('Should return 200 response - GET /ponyfill', async () => {
const res = await request(server).get('/ponyfill')
expect(res.status).toBe(200)
expect(res.headers['content-type']).toMatch('text/plain')
expect(res.text).toBe('Pony')
})

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

Great! Looks good to me!

@yusukebe yusukebe merged commit 12f75e0 into honojs:main Feb 17, 2024
3 checks passed
nicolewhite referenced this pull request in autoblocksai/cli Mar 20, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [@hono/node-server](https://togithub.com/honojs/node-server) |
[`1.7.0` ->
`1.8.2`](https://renovatebot.com/diffs/npm/@hono%2fnode-server/1.7.0/1.8.2)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/@hono%2fnode-server/1.8.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@hono%2fnode-server/1.8.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@hono%2fnode-server/1.7.0/1.8.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@hono%2fnode-server/1.7.0/1.8.2?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>honojs/node-server (@&#8203;hono/node-server)</summary>

###
[`v1.8.2`](https://togithub.com/honojs/node-server/releases/tag/v1.8.2)

[Compare
Source](https://togithub.com/honojs/node-server/compare/v1.8.1...v1.8.2)

#### What's Changed

- fix: duplex only has a getter by
[@&#8203;wenerme](https://togithub.com/wenerme) in
[https://github.com/honojs/node-server/pull/149](https://togithub.com/honojs/node-server/pull/149)

#### New Contributors

- [@&#8203;wenerme](https://togithub.com/wenerme) made their first
contribution in
[https://github.com/honojs/node-server/pull/149](https://togithub.com/honojs/node-server/pull/149)

**Full Changelog**:
honojs/node-server@v1.8.1...v1.8.2

###
[`v1.8.1`](https://togithub.com/honojs/node-server/releases/tag/v1.8.1)

[Compare
Source](https://togithub.com/honojs/node-server/compare/v1.8.0...v1.8.1)

This release includes a `feat` change, but it's minor, so release this
as a patch-release.

#### What's Changed

- feat: support keepalive property for Request object by
[@&#8203;usualoma](https://togithub.com/usualoma) in
[https://github.com/honojs/node-server/pull/148](https://togithub.com/honojs/node-server/pull/148)

**Full Changelog**:
honojs/node-server@v1.8.0...v1.8.1

###
[`v1.8.0`](https://togithub.com/honojs/node-server/releases/tag/v1.8.0)

[Compare
Source](https://togithub.com/honojs/node-server/compare/v1.7.0...v1.8.0)

#### What's Changed

- feat: bump hono to v3.12.8, add customer err tests for compress middl…
by [@&#8203;ariskemper](https://togithub.com/ariskemper) in
[https://github.com/honojs/node-server/pull/130](https://togithub.com/honojs/node-server/pull/130)
- refactor(test): replace features to be removed in v4 by
[@&#8203;ryuapp](https://togithub.com/ryuapp) in
[https://github.com/honojs/node-server/pull/136](https://togithub.com/honojs/node-server/pull/136)
- Catch when nodejs request is aborted by
[@&#8203;M4RC3L05](https://togithub.com/M4RC3L05) in
[https://github.com/honojs/node-server/pull/141](https://togithub.com/honojs/node-server/pull/141)
- chore: bump hono to v4 by
[@&#8203;yusukebe](https://togithub.com/yusukebe) in
[https://github.com/honojs/node-server/pull/143](https://togithub.com/honojs/node-server/pull/143)
- feat: use internal body if available for returning the response in its
original form as much as possible by
[@&#8203;usualoma](https://togithub.com/usualoma) in
[https://github.com/honojs/node-server/pull/145](https://togithub.com/honojs/node-server/pull/145)

#### New Contributors

- [@&#8203;ariskemper](https://togithub.com/ariskemper) made their first
contribution in
[https://github.com/honojs/node-server/pull/130](https://togithub.com/honojs/node-server/pull/130)
- [@&#8203;ryuapp](https://togithub.com/ryuapp) made their first
contribution in
[https://github.com/honojs/node-server/pull/136](https://togithub.com/honojs/node-server/pull/136)
- [@&#8203;M4RC3L05](https://togithub.com/M4RC3L05) made their first
contribution in
[https://github.com/honojs/node-server/pull/141](https://togithub.com/honojs/node-server/pull/141)

**Full Changelog**:
honojs/node-server@v1.7.0...v1.8.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on Monday" in timezone
America/Chicago, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/autoblocksai/cli).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNDUuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI0NS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

should use writeFromReadableStream() for a ReadableStream
2 participants