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.

Sign up for GitHub

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

fix(fetch): respect Request properties #688

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

fix(fetch): respect Request properties #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c488a8
fix(fetch): respect `Request` properties
balazsorban44 Nov 2, 2023
656c1f3
fix
balazsorban44 Nov 2, 2023
5c11df4
don't set init to `true`
balazsorban44 Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/thick-laws-marry.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@edge-runtime/primitives': patch
---

If `fetch` gets a `Request` passed as the first argument, respect the `method`, `headers`, `body` and `credentials` properties
41 changes: 36 additions & 5 deletions packages/integration-tests/test/fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const testOrSkip =
process.versions.node.split('.').map(Number)[0] > 16 ? test : test.skip

testOrSkip('perform a GET', async () => {
server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
server = createServer(async (req, res) => {
if (req.method !== 'GET') {
res.statusCode = 400
res.end()
Expand All @@ -30,7 +30,7 @@ testOrSkip('perform a GET', async () => {
})

testOrSkip('perform a POST as application/json', async () => {
server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
server = createServer(async (req, res) => {
if (req.method !== 'POST') {
res.statusCode = 400
res.end()
Expand Down Expand Up @@ -62,7 +62,7 @@ testOrSkip('perform a POST as application/json', async () => {
})

testOrSkip('perform a POST as application/x-www-form-urlencoded', async () => {
server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
server = createServer(async (req, res) => {
if (req.method !== 'POST') {
res.statusCode = 400
res.end()
Expand Down Expand Up @@ -98,7 +98,7 @@ testOrSkip('perform a POST as application/x-www-form-urlencoded', async () => {

testOrSkip('perform a POST as multipart/form-data', async () => {
const upload = multer({ storage: multer.memoryStorage() })
server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
server = createServer(async (req, res) => {
if (
req.method !== 'POST' ||
!req.headers['content-type']?.startsWith('multipart/form-data')
Expand Down Expand Up @@ -133,7 +133,7 @@ testOrSkip('perform a POST as multipart/form-data', async () => {
})

testOrSkip('sets header calling Headers constructor', async () => {
server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
server = createServer(async (req, res) => {
res.end(req.headers['user-agent'])
})
const serverUrl = await listen(server)
Expand All @@ -157,3 +157,34 @@ testOrSkip('sets header calling Headers constructor', async () => {
expect(response.status).toBe(200)
},
)

testOrSkip.only('respect Request instance options', async () => {
server = createServer(async (req, res) => {
if (req.method !== 'POST') {
res.statusCode = 400
res.end()
}

if (req.headers['content-type'] === 'application/json') {
const text = await httpBody.text(req)
res.statusCode = 200
res.setHeader('Content-Type', 'application/json')
res.setHeader('Content-Length', Buffer.byteLength(text))
res.end(text)
return
}
res.statusCode = 400
res.end()
})

const serverUrl = await listen(server)
const request = new Request(new Request(serverUrl), {
headers: { 'content-type': 'application/json' },
})
const response = await fetch(request, {
body: JSON.stringify({ foo: 'bar' }),
})

expect(response.status).toBe(200)
expect(await response.json()).toEqual({ foo: 'bar' })
})
8 changes: 8 additions & 0 deletions packages/primitives/src/primitives/fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,17 @@ function addDuplexToInit(init) {
* Export fetch with an implementation that uses a default global dispatcher.
* It also re-cretates a new Response object in order to allow mutations on
* the Response headers.
* @param {RequestInfo} info
* @param {RequestInit} init
*/
export async function fetch(info, init) {
init = addDuplexToInit(init)
if (info instanceof Request) {
init.method ??= info.method
Copy link

Choose a reason for hiding this comment

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

I am not sure how far long this PR is, so the comment may be premature, but two thoughts:

  1. why are only these particular properties of Request are copied over. It has other many other properties which are also valid to be set via the second argument to fetch. E.g. mode, cache, and others.
  2. I am not sure how this fixes Cloning a Request in edge runtime when running the dev server does not work correctly next.js#57905. That issue is about the Request constructor not working properly when the first argument to it is a Request instance - fetch is not involved at all, though it is in some ways similar.

init.headers ??= info.headers
init.body ??= info.body
init.credentials ??= info.credentials
}
const res = await fetchImpl.call(getGlobalDispatcher(), info, init)
const response = new Response(res.body, res)
Object.defineProperty(response, 'url', { value: res.url })
Expand Down