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

Simplify FetchRequestDelegate.prepareHeadersForRequest #798

Merged
merged 1 commit into from
Nov 27, 2022
Merged

Simplify FetchRequestDelegate.prepareHeadersForRequest #798

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/core/drive/form_submission.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { FetchRequest, FetchMethod, fetchMethodFromString, FetchRequestHeaders } from "../../http/fetch_request"
import { FetchRequest, FetchMethod, fetchMethodFromString } from "../../http/fetch_request"
import { FetchResponse } from "../../http/fetch_response"
import { expandURL } from "../url"
import { dispatch, getAttribute, getMetaContent, hasAttribute } from "../../util"
Expand Down Expand Up @@ -149,11 +149,11 @@ export class FormSubmission {

// Fetch request delegate

prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) {
prepareRequest(request: FetchRequest) {
if (!request.isIdempotent) {
const token = getCookieValue(getMetaContent("csrf-param")) || getMetaContent("csrf-token")
if (token) {
headers["X-CSRF-Token"] = token
request.headers["X-CSRF-Token"] = token
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/core/drive/visit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Adapter } from "../native/adapter"
import { FetchMethod, FetchRequest, FetchRequestDelegate, FetchRequestHeaders } from "../../http/fetch_request"
import { FetchMethod, FetchRequest, FetchRequestDelegate } from "../../http/fetch_request"
import { FetchResponse } from "../../http/fetch_response"
import { History } from "./history"
import { getAnchor } from "../url"
Expand Down Expand Up @@ -335,7 +335,7 @@ export class Visit implements FetchRequestDelegate {

// Fetch request delegate

prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) {
prepareRequest(request: FetchRequest) {
if (this.acceptsStreamResponse) {
request.acceptResponseType(StreamMessage.contentType)
}
Expand Down
8 changes: 4 additions & 4 deletions src/core/frames/frame_controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
FrameLoadingStyle,
FrameElementObservedAttribute,
} from "../../elements/frame_element"
import { FetchMethod, FetchRequest, FetchRequestDelegate, FetchRequestHeaders } from "../../http/fetch_request"
import { FetchMethod, FetchRequest, FetchRequestDelegate } from "../../http/fetch_request"
import { FetchResponse } from "../../http/fetch_response"
import { AppearanceObserver, AppearanceObserverDelegate } from "../../observers/appearance_observer"
import {
Expand Down Expand Up @@ -238,14 +238,14 @@ export class FrameController

this.formSubmission = new FormSubmission(this, element, submitter)
const { fetchRequest } = this.formSubmission
this.prepareHeadersForRequest(fetchRequest.headers, fetchRequest)
this.prepareRequest(fetchRequest)
this.formSubmission.start()
}

// Fetch request delegate

prepareHeadersForRequest(headers: FetchRequestHeaders, request: FetchRequest) {
headers["Turbo-Frame"] = this.id
prepareRequest(request: FetchRequest) {
request.headers["Turbo-Frame"] = this.id

if (this.currentNavigationElement?.hasAttribute("data-turbo-stream")) {
request.acceptResponseType(StreamMessage.contentType)
Expand Down
4 changes: 2 additions & 2 deletions src/http/fetch_request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export type TurboFetchRequestErrorEvent = CustomEvent<{
export interface FetchRequestDelegate {
referrer?: URL

prepareHeadersForRequest?(headers: FetchRequestHeaders, request: FetchRequest): void
prepareRequest(request: FetchRequest): void
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍 I agree that this makes it simpler. As you say, this method no longer needs to be optional, given that every delegate implements it.

The headers are still the only part that gets modified at this stage. So if we wanted to keep the purpose of the method the same, another idea could be to move the acceptResponseType helper out of FetchRequest, and have it operate on a FetchRequestHeaders instance instead. That way the prepareHeadersForRequest method name still works. So something more like

acceptResponseType(headers, StreamMessage.contentType)

But, I like the way you have it here. Logically this method is used to "prepare the request", regardless of which specific properties are being set, so the more general name makes sense.

requestStarted(request: FetchRequest): void
requestPreventedHandlingResponse(request: FetchRequest, response: FetchResponse): void
requestSucceededWithResponse(request: FetchRequest, response: FetchResponse): void
Expand Down Expand Up @@ -103,7 +103,7 @@ export class FetchRequest {

async perform(): Promise<FetchResponse | void> {
const { fetchOptions } = this
this.delegate.prepareHeadersForRequest?.(this.headers, this)
this.delegate.prepareRequest(this)
await this.allowRequestToBeIntercepted(fetchOptions)
try {
this.delegate.requestStarted(this)
Expand Down