Prepare Frame Form Submission fetch headers

[seanpdoyle]

Closes #86
Closes #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 [email protected]

[seanpdoyle]

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()?

[tleish]

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);
+     }
    }
  }

[seanpdoyle]

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.

[tleish]

Agreed.

[sstephenson]

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 {

[sstephenson]

Restore the defaultHeaders definition and use that to assign the initial value of headers:

Suggested change

	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.

[sstephenson]

Excellent, thank you!