Skip to content

Don't convert data-turbo-stream links to forms#647

Merged
dhh merged 1 commit intohotwired:mainfrom
kevinmcconnell:stream-links-without-forms
Aug 1, 2022
Merged

Don't convert data-turbo-stream links to forms#647
dhh merged 1 commit intohotwired:mainfrom
kevinmcconnell:stream-links-without-forms

Conversation

@kevinmcconnell
Copy link
Copy Markdown
Collaborator

@kevinmcconnell kevinmcconnell commented Jul 25, 2022

The initial implementation of the data-turbo-stream attribute worked
by treating requests with that attribute as form submissions (which
would be GET form submissions, unless a different method type was
indicated).

This allowed us to include the data-turbo-stream logic in
FormSubmission, which was responsible for properly setting the
Accept header.

However, there are some downsides to submitting the requests as form
submissions. For example, a GET form submission does not include the
search portion of the URL. Additionally, servers are free to respond to
data-turbo-stream requests with plain HTML responses, and in that case
we don't want Turbo's handling of the response to differ from what it
would have been if the data-turbo-stream attribute wasn't present.

This commit takes a different approach. In this version, elements that
have data-turbo-stream continue to be processed by the same object
that they would otherwise, whether that's a FormSubmission, a Visit
or a FrameController. However each of these objects is now aware of
the data-turbo-stream attribute, and each will include the Turbo
Stream MIME type when appropriate.

This minimizes the impact of data-turbo-stream so that the only effect
it has is on the inclusion of that MIME type.

kevinmcconnell
kevinmcconnell commented Jul 25, 2022
View reviewed changes
src/observers/form_link_interceptor.ts Outdated
this.delegate.shouldInterceptFormLinkClick(link) &&
(link.hasAttribute("data-turbo-method") || link.hasAttribute("data-turbo-stream"))
)
return this.delegate.shouldInterceptFormLinkClick(link) && link.hasAttribute("data-turbo-method")
Copy link
Copy Markdown
Collaborator Author

@kevinmcconnell kevinmcconnell Jul 25, 2022

Choose a reason for hiding this comment

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

Removing the check for data-turbo-stream here is what removes the previous behaviour of turning data-turbo-stream links into GET form submissions.

kevinmcconnell
kevinmcconnell commented Jul 25, 2022
View reviewed changes
src/tests/functional/form_submission_tests.ts
assert.ok(fetchOptions.headers["Accept"].includes("text/vnd.turbo-stream.html"))
})

test("test stream link outside frame", async ({ page }) => {
Copy link
Copy Markdown
Collaborator Author

@kevinmcconnell kevinmcconnell Jul 25, 2022

Choose a reason for hiding this comment

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

As links like this are no longer treated as form submissions, I've moved this to the visit tests.

@kevinmcconnell
Copy link
Copy Markdown
Collaborator Author

kevinmcconnell commented Jul 25, 2022

This is an alternative approach to fixing the issue in #641 (as discussed on that PR), and also generally improves on #612.

@seanpdoyle I finally found some time to finish this up after we discussed it last week. Would you mind taking a look if you get a chance? Thanks!

@kevinmcconnell
Copy link
Copy Markdown
Collaborator Author

kevinmcconnell commented Jul 27, 2022

@dhh could we include this in the 7.2.0 milestone? I think this a better implementation of data-turbo-stream, and it would be nice to get the extra testing on this version before shipping.

@dhh dhh added this to the 7.2.0 milestone Jul 28, 2022
@dhh
Copy link
Copy Markdown
Member

dhh commented Jul 29, 2022

Can you rebase?

@kevinmcconnell
Copy link
Copy Markdown
Collaborator Author

kevinmcconnell commented Jul 30, 2022

@dhh this has been rebased now.

@kevinmcconnell
Don't convert data-turbo-stream links to forms
3770b3f 
The initial implementation of the `data-turbo-stream` attribute worked
by treating requests with that attribute as form submissions (which
would usually be `GET` form submissions, unless a different method type
was indicated).

This allowed us to include the `data-turbo-stream` logic in
`FormSubmission`, which was responsible for properly setting the
`Accept` header.

However, there are some downsides to submitting the requests as form
submissions. For example, a `GET` form submission does not include the
search portion of the URL. Additionally, servers are free to respond to
`data-turbo-stream` requests with plain HTML responses, and in that case
we don't want Turbo's handling of the response to differ from what it
would have been if the `data-turbo-stream` attribute wasn't present.

This commit takes a different approach. In this version, elements that
have `data-turbo-stream` continue to be processed by the same object
that they would otherwise, whether that's a `FormSubmission`, a `Visit`
or a `FrameController`. However each of these objects is now aware of
the `data-turbo-stream` attribute, and each will include the Turbo
Stream MIME type when appropriate.

This minimizes the impact of `data-turbo-stream` so that the only effect
it has is on the inclusion of that MIME type.
@kevinmcconnell
Copy link
Copy Markdown
Collaborator Author

kevinmcconnell commented Aug 1, 2022

You can ignore the extra couple of force pushes here -- was just triggering CI to get a clean test run. It looks like we have a couple of flaky tests on main at the moment.

@dhh dhh merged commit bbd8053 into hotwired:main Aug 1, 2022
@dhh dhh mentioned this pull request Aug 1, 2022
Closed
dhh added a commit to seanpdoyle/turbo that referenced this pull request Aug 3, 2022
@dhh
Merge branch 'main' into pr/445
52c9ced 
* main:
  Add turbo:fetch-request-error event on frame and form network errors (hotwired#640)
  Return `Promise<void>` from `FrameElement.reload` (hotwired#661)
  Replace LinkInterceptor with LinkClickObserver (hotwired#412)
  Don't convert `data-turbo-stream` links to forms (hotwired#647)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Nov 20, 2022
@seanpdoyle
Simplify FetchRequestDelegate.prepareHeadersForRequest
76c69bb 
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 `prepareForRequest`, 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 added a commit to seanpdoyle/turbo that referenced this pull request Nov 20, 2022
@seanpdoyle
Simplify FetchRequestDelegate.prepareHeadersForRequest
709afe1 
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 added a commit to seanpdoyle/turbo that referenced this pull request Nov 20, 2022
@seanpdoyle
Simplify FetchRequestDelegate.prepareHeadersForRequest
6de2ad4 
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
dhh pushed a commit that referenced this pull request Nov 27, 2022
@seanpdoyle
Simplify FetchRequestDelegate.prepareHeadersForRequest (#798)
2b41a93 
With the introduction of `FetchRequest.acceptResponseType()` in
[#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.

[#647]: https://github.com/hotwired/turbo/pull/647/files#diff-d4ee4683f7121e24a87566ef6854ee6090ee723a5299430338f5602febea8c1f
Challenge-Guy pushed a commit to Challenge-Guy/turbo-cfm1 that referenced this pull request Mar 8, 2025
@seanpdoyle
Simplify FetchRequestDelegate.prepareHeadersForRequest (#798)
5b7affb 
With the introduction of `FetchRequest.acceptResponseType()` in
[hotwired/turbo#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/turbo#647]: https://github.com/hotwired/turbo/pull/647/files#diff-d4ee4683f7121e24a87566ef6854ee6090ee723a5299430338f5602febea8c1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

2 participants

@kevinmcconnell @dhh