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.

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

Override FetchOptions from event listeners #691

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Aug 17, 2022

The problem

Prior to this commit, instances of FormSubmission and FetchRequest
referenced in dispatched turbo:submit-start and
turbo:before-fetch-request events were tedious to modify from a host
application.

For example, the FormSubmission instance made available through
event.detail.formSubmission exposed properties that seemed writable.

Unfortunately, any mutations to formSubmission.method or
formSubmission.location from within host application event listeners
(like turbo:before-fetch-request) occur after the underlying
FetchRequest instance is built, which has subtle and not obvious
implications. In practice, modifications to these properties are
unobserved by the underlying FetchRequest instance, and have no effect
on the ensuing fetch call.

Luckily, if host applications are aware of these issues and limitations,
they can work around them by mutating the
event.detail.formSubission.fetchRequest instance.

The proposal

After this commit, the FormSubmission.constructor will infer the
required data (like [method] and [action] values) from its
HTMLFormElement and optional HTMLElement arguments, then use those
values to construct the FetchRequest instance.

After that construction, any changes to the FormSubmission properties
will delegate to the FetchRequest instance.

In tandem with those changes, the FetchRequest will follow a similar
pattern: extract as much information up-front during construction, then
delegate property reads and writes to properties. In most
cases, it will delegate to its fetchOptions: RequestInit instance.

To achieve this behavior, several module-local functions and type
definitions were shuffled around between the FormSubmission and
FetchRequest modules. The order of arguments for the FetchRequest
constructor were also re-arranged to support omitting values that are
made redundant by default arguments.

@seanpdoyle seanpdoyle force-pushed the request-event-listeners branch 4 times, most recently from 31d7816 to d0a147e Compare August 17, 2022 20:46
@seanpdoyle seanpdoyle force-pushed the request-event-listeners branch 8 times, most recently from 3a8c75e to d863abf Compare September 14, 2022 19:54
@seanpdoyle seanpdoyle force-pushed the request-event-listeners branch 2 times, most recently from 92be277 to ca83668 Compare September 27, 2022 14:04
@seanpdoyle seanpdoyle force-pushed the request-event-listeners branch 2 times, most recently from 049fef3 to dc5ea66 Compare December 31, 2022 23:05
@seanpdoyle seanpdoyle force-pushed the request-event-listeners branch 4 times, most recently from b0ba4fa to a407922 Compare September 10, 2023 20:30
The problem
---

Prior to this commit, instances of `FormSubmission` and `FetchRequest`
referenced in dispatched `turbo:submit-start` and
`turbo:before-fetch-request` events were tedious to update.

For example, the `FormSubmission` instance made available through
`event.detail.formSubmission` exposed properties that _seemed_ readable
and writable.

However, mutations to `formSubmission.method` or
`formSubmission.location` occur _after_ the underlying `FetchRequest`
instance is built. This timing of that instance's construction has
subtle and not obvious implications. In practice, writes to these
properties are unobserved by the underlying `FetchRequest` instance, and
have no effect on the `fetch` call.

If calling applications are aware of these issues and limitations, they
can work around them by mutating the
`event.detail.formSubission.fetchRequest` instance.

The proposal
---

After this commit, the `FormSubmission.constructor` will deduce the
required data (like `[method]` and `[action]` values) then use those
values to construct the `FetchRequest` instance. After that
construction, the reads and writes to the `FormSubmission` properties
will _delegate_ to the `FetchRequest` instance.

In tandem with those changes, the `FetchRequest` will follow a similar
pattern: extract as much information up-front during construction, then
delegate property reads and writes to the correct instance. In most
cases, it will delegate to its [`fetchOptions:
RequestInit`][RequestInit] instance.

To achieve this behavior, several module-local functions and type
definitions were shuffled around between the `FormSubmission` and
`FetchRequest` modules. The order of arguments for the `FetchRequest`
constructor were also re-arranged to support omitting values that are
made redundant by default arguments.

[RequestInit]: https://developer.mozilla.org/en-US/docs/Web/API/fetch#parameters
@seanpdoyle
Copy link
Contributor Author

@afcapel @kevinmcconnell I've rebased this branch to account for the removal of types. It's ready for review.

@afcapel afcapel merged commit d9821c4 into hotwired:main Sep 12, 2023
1 check passed
@afcapel
Copy link
Collaborator

afcapel commented Sep 12, 2023

@seanpdoyle looks good, clever refactor! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants