Simplify FetchRequestDelegate.prepareHeadersForRequest
#798
Conversation
With the introduction of `FetchRequest.acceptResponseType()` in [hotwired#647][], the `FetchRequestDelegate.prepareHeadersForRequest` callback interacts with more than just the `FetchRequestHeaders` instance passes as its first argument. Additionally, every implementation of the `FetchRequestDelegate` interface implements the `prepareHeadersForRequest` method, so the fact that it's listed as optional and invoked with a guard clause are unnecessary. This commit renames the method to `prepareRequest`, reduces the signature to a single `FetchRequest` argument, and no longer declares it as a conditional property on the interface. [hotwired#647]: https://github.com/hotwired/turbo/pull/647/files#diff-d4ee4683f7121e24a87566ef6854ee6090ee723a5299430338f5602febea8c1f
seanpdoyle
force-pushed
the
prepare-request-delegate
branch
from
709afe1 to
6de2ad4
Compare
|
Hey @kevinmcconnell, this came out of some of the Turbo Stream work you've done. If you're available, I'd really appreciate some code review! |
| @@ -18,7 +18,7 @@ export type TurboFetchRequestErrorEvent = CustomEvent<{ | |||
| export interface FetchRequestDelegate { | |||
| referrer?: URL | |||
|
|
|||
| prepareHeadersForRequest?(headers: FetchRequestHeaders, request: FetchRequest): void | |||
| prepareRequest(request: FetchRequest): void | |||
There was a problem hiding this comment.
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.
With the introduction of
FetchRequest.acceptResponseType()in hotwired/turbo#647, theFetchRequestDelegate.prepareHeadersForRequestcallback interacts with more than just theFetchRequestHeadersinstance passes as its first argument.Additionally, every implementation of the
FetchRequestDelegateinterface implements theprepareHeadersForRequestmethod, so the fact that it's listed as optional and invoked with a guard clause are unnecessary.This commit renames the method to
prepareRequest, reduces the signature to a singleFetchRequestargument, and no longer declares it as a conditional property on the interface.