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

Jump to bottom

Prepare Frame Form Submission fetch headers #166

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Prepare Frame Form Submission fetch headers #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/core/frames/frame_controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ export class FrameController implements AppearanceObserverDelegate, FetchRequest
if (this.formSubmission.fetchRequest.isIdempotent) {
this.navigateFrame(element, this.formSubmission.fetchRequest.url.href)
} else {
const { fetchRequest } = this.formSubmission
this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@seanpdoyle seanpdoyle Feb 7, 2021 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

this.formSubmission.start()
}
}
Expand Down
23 changes: 9 additions & 14 deletions src/http/fetch_request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,15 @@ export interface FetchRequestOptions {
export class FetchRequest {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 {

readonly delegate: FetchRequestDelegate
readonly method: FetchMethod
readonly headers: FetchRequestHeaders
readonly url: URL
readonly body?: FetchRequestBody
readonly abortController = new AbortController

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 {
Expand All @@ -75,6 +77,7 @@ export class FetchRequest {

async perform(): Promise<FetchResponse> {
const { fetchOptions } = this
this.delegate.prepareHeadersForRequest?.(this.headers, this)
dispatch("turbo:before-fetch-request", { detail: { fetchOptions } })
try {
this.delegate.requestStarted(this)
Expand Down Expand Up @@ -112,27 +115,19 @@ export class FetchRequest {
}
}

get isIdempotent() {
return this.method == FetchMethod.get
get defaultHeaders() {
return {
"Accept": "text/html, application/xhtml+xml"
}
}

get headers() {
const headers = { ...this.defaultHeaders }
if (typeof this.delegate.prepareHeadersForRequest == "function") {
this.delegate.prepareHeadersForRequest(headers, this)
}
return headers
get isIdempotent() {
return this.method == FetchMethod.get
}

get abortSignal() {
return this.abortController.signal
}

get defaultHeaders() {
return {
"Accept": "text/html, application/xhtml+xml"
}
}
}

function mergeFormDataEntries(url: URL, entries: [string, FormDataEntryValue][]): URL {
Expand Down
16 changes: 16 additions & 0 deletions src/tests/functional/form_submission_tests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,14 @@ export class FormSubmissionTests extends TurboDriveTestCase {
this.assert.equal(htmlAfter, htmlBefore)
}

async "test frame form submission within a frame submits the Turbo-Frame header"() {
await this.clickSelector("#frame form.redirect input[type=submit]")

const { fetchOptions } = await this.nextEventNamed("turbo:before-fetch-request")

this.assert.ok(fetchOptions.headers["Turbo-Frame"], "submits with the Turbo-Frame header")
}

async "test invalid frame form submission with unprocessable entity status"() {
await this.clickSelector("#frame form.unprocessable_entity input[type=submit]")
await this.nextBeat
Expand Down Expand Up @@ -298,6 +306,14 @@ export class FormSubmissionTests extends TurboDriveTestCase {
this.assert.equal(await this.pathname, "/src/tests/fixtures/one.html")
}

async "test form submission targeting a frame submits the Turbo-Frame header"() {
await this.clickSelector('#targets-frame [type="submit"]')

const { fetchOptions } = await this.nextEventNamed("turbo:before-fetch-request")

this.assert.ok(fetchOptions.headers["Turbo-Frame"], "submits with the Turbo-Frame header")
}

get formSubmitted(): Promise<boolean> {
return this.hasSelector("html[data-form-submitted]")
}
Expand Down