-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(routing): don't trigger get of headers #12937
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'astro': patch | ||
--- | ||
|
||
Fixes a bug where users could use `Astro.request.headers` during a rewrite inside prerendered routes. This an invalid behaviour, and now Astro will show a warning if this happens. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'astro': patch | ||
--- | ||
|
||
Fixes an issue where the use of `Astro.rewrite` would trigger the invalid use of `Astro.request.headers` | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,22 @@ | ||
import type { IncomingHttpHeaders } from 'node:http'; | ||
import { AstroError, AstroErrorData } from './errors/index.js'; | ||
import type { Logger } from './logger/core.js'; | ||
|
||
type HeaderType = Headers | Record<string, any> | IncomingHttpHeaders; | ||
type RequestBody = ArrayBuffer | Blob | ReadableStream | URLSearchParams | FormData; | ||
type RequestBody = | ||
| ArrayBuffer | ||
| Blob | ||
| ReadableStream | ||
| URLSearchParams | ||
| FormData | ||
| ReadableStream<Uint8Array>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is type provided by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry @florian-lefebvre, I completely missed your comment. I'll follow up with another PR |
||
|
||
export interface CreateRequestOptions { | ||
url: URL | string; | ||
clientAddress?: string | undefined; | ||
headers: HeaderType; | ||
method?: string; | ||
body?: RequestBody | undefined; | ||
body?: RequestBody | undefined | null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added |
||
logger: Logger; | ||
locals?: object | undefined; | ||
/** | ||
|
@@ -22,6 +29,8 @@ export interface CreateRequestOptions { | |
isPrerendered?: boolean; | ||
|
||
routePattern: string; | ||
|
||
init?: RequestInit; | ||
} | ||
|
||
/** | ||
|
@@ -39,6 +48,7 @@ export function createRequest({ | |
logger, | ||
isPrerendered = false, | ||
routePattern, | ||
init, | ||
}: CreateRequestOptions): Request { | ||
// headers are made available on the created request only if the request is for a page that will be on-demand rendered | ||
const headersObj = isPrerendered | ||
|
@@ -65,6 +75,7 @@ export function createRequest({ | |
headers: headersObj, | ||
// body is made available only if the request is for a page that will be on-demand rendered | ||
body: isPrerendered ? null : body, | ||
...init, | ||
}); | ||
|
||
if (isPrerendered) { | ||
|
@@ -92,3 +103,32 @@ export function createRequest({ | |
|
||
return request; | ||
} | ||
|
||
/** | ||
* Utility function that creates a new `Request` with a new URL from an old `Request`. | ||
* | ||
* @param newUrl The new `URL` | ||
* @param oldRequest The old `Request` | ||
*/ | ||
export function copyRequest(newUrl: URL, oldRequest: Request, isPrerendered: boolean): Request { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this version of the function used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bad copy-paste. Good catch |
||
if (oldRequest.bodyUsed) { | ||
throw new AstroError(AstroErrorData.RewriteWithBodyUsed); | ||
} | ||
return new Request(newUrl, { | ||
method: oldRequest.method, | ||
headers: isPrerendered ? {} : oldRequest.headers, | ||
body: oldRequest.body, | ||
referrer: oldRequest.referrer, | ||
referrerPolicy: oldRequest.referrerPolicy, | ||
mode: oldRequest.mode, | ||
credentials: oldRequest.credentials, | ||
cache: oldRequest.cache, | ||
redirect: oldRequest.redirect, | ||
integrity: oldRequest.integrity, | ||
signal: oldRequest.signal, | ||
keepalive: oldRequest.keepalive, | ||
// https://fetch.spec.whatwg.org/#dom-request-duplex | ||
// @ts-expect-error It isn't part of the types, but undici accepts it and it allows to carry over the body to a new request | ||
duplex: 'half', | ||
}); | ||
} |
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.
Are these two separate issues beign fixed? It's a little unclear.
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.
Yes. What should I reword?
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.
That's fine. Just wanted to confirm that they're separate.