Prepare Frame Form Submission fetch headers#166
Conversation
44eda99 to
1fa60a6
Compare
8776d38 to
1e529e7
Compare
1e529e7 to
a6fe71d
Compare
| this.navigateFrame(element, this.formSubmission.fetchRequest.url.href) | ||
| } else { | ||
| const { fetchRequest } = this.formSubmission | ||
| this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest) |
There was a problem hiding this comment.
My main instinct was to prefer a more explicit, layered mutation over the recursive style proposed by https://github.com/hotwired/turbo/pull/110/files#diff-68b647dc2963716dc27c070f261d0b942ee9c00be7c4149ecb3a5acd94842d40, but the more I pulled on that thread, the more changes it required to stick.
Is there an alternative delegator pattern that could still leverage the late-binding values that the get headers() property provides without recursively walking a chain of objects that coincidentally declare prepareHeadersForRequest()?
There was a problem hiding this comment.
the only impact we are aware of right now involves forms. We could consider isolating the change to forms only. Something like:
src/core/drive/form_submission.ts
prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) {
if (!request.isIdempotent) {
const token = getCookieValue(getMetaContent("csrf-param")) || getMetaContent("csrf-token")
if (token) {
headers["X-CSRF-Token"] = token
}
headers["Accept"] = [ StreamMessage.contentType, headers["Accept"] ].join(", ")
+ if (typeof this.delegate.prepareHeadersForRequest == "function") {
+ this.delegate.prepareHeadersForRequest(headers, request);
+ }
}
}There was a problem hiding this comment.
That's a good point. What bothers me the most is that checking that this.delegate (a FormSubmissionDelegate instance) is of a different interface than the FetchRequestDelegate. The fact that they both declare prepareHeadersForRequest is a coincidence that signals to me that there's some more Typescript-driven design thinking we need to do to chain these things together in a more type-aware manner.
The change in this PR is at least more explicit in that way: FrameController has direct access to a FormSubmission instance, which itself has direct access to a FetchRequest. It's access that circumvents the delegator pattern, but it at least interacts with properties that are type checked and are part of the intermediate objects' public interfaces.
| @@ -42,6 +42,7 @@ export interface FetchRequestOptions { | |||
| export class FetchRequest { | |||
There was a problem hiding this comment.
The changes to this file can be simplified by asking the delegate to prepare headers in perform() just before dispatching the turbo:before-fetch-request event.
diff --git a/src/http/fetch_request.ts b/src/http/fetch_request.ts
index 532ea3e..b4205a2 100644
--- a/src/http/fetch_request.ts
+++ b/src/http/fetch_request.ts
@@ -50,13 +50,13 @@ export class FetchRequest {
constructor(delegate: FetchRequestDelegate, method: FetchMethod, location: URL, body: FetchRequestBody = new URLSearchParams) {
this.delegate = delegate
this.method = method
+ this.headers = this.defaultHeaders
if (this.isIdempotent) {
this.url = mergeFormDataEntries(location, [ ...body.entries() ])
} else {
this.body = body
this.url = location
}
- this.headers = prepareHeadersForRequest(this)
}
get location(): URL {
@@ -77,8 +77,9 @@ export class FetchRequest {
async perform(): Promise<FetchResponse> {
const { fetchOptions } = this
- dispatch("turbo:before-fetch-request", { detail: { fetchOptions } })
try {
+ this.delegate.prepareHeadersForRequest?.(this.headers, this)
+ dispatch("turbo:before-fetch-request", { detail: { fetchOptions } })
this.delegate.requestStarted(this)
const response = await fetch(this.url.href, fetchOptions)
return await this.receive(response)
@@ -121,14 +122,12 @@ export class FetchRequest {
get abortSignal() {
return this.abortController.signal
}
-}
-function prepareHeadersForRequest(fetchRequest: FetchRequest) {
- const headers = { "Accept": "text/html, application/xhtml+xml" }
- if (typeof fetchRequest.delegate.prepareHeadersForRequest == "function") {
- fetchRequest.delegate.prepareHeadersForRequest(headers, fetchRequest)
+ get defaultHeaders() {
+ return {
+ "Accept": "text/html, application/xhtml+xml"
+ }
}
- return headers
}
function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL {
src/http/fetch_request.ts
Outdated
| this.body = body | ||
| this.url = location | ||
| } | ||
| this.headers = prepareHeadersForRequest(this) |
There was a problem hiding this comment.
Restore the defaultHeaders definition and use that to assign the initial value of headers:
| this.headers = prepareHeadersForRequest(this) | |
| this.headers = this.defaultHeaders |
And let's move this line up to just after the this.method assignment for symmetry with the property declarations.
a6fe71d to
4e6b826
Compare
Closes hotwired#86 Closes hotwired#110 When submitting a Form that is within a `<turbo-frame>` or targets a `<turbo-frame>`, ensure that the `Turbo-Frame` header is present. Since the constructive-style `FetchRequestDelegate.additionalHeadersForRequest()` was replaced by the mutative style `FetchRequestDelegate.prepareHeadersForRequest()`, this commit introduces a readonly `FetchRequestHeaders` property created at constructor-time so that its values can be mutated prior to the request's submission. Co-authored-by: tleish <tleish@users.noreply.github.com>
4e6b826 to
5f9a8e8
Compare
|
Excellent, thank you! |
Closes #86
Closes #110
When submitting a Form that is within a
<turbo-frame>or targets a<turbo-frame>, ensure that theTurbo-Frameheader is present.Since the constructive-style
FetchRequestDelegate.additionalHeadersForRequest()was replaced by themutative style
FetchRequestDelegate.prepareHeadersForRequest(), thiscommit introduces a readonly
FetchRequestHeadersproperty created atconstructor-time so that its values can be mutated prior to the
request's submission.
Co-authored-by: tleish tleish@users.noreply.github.com