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

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

Merged
merged 1 commit into from
Aug 1, 2022
Merged

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

merged 1 commit into from
Aug 1, 2022

Conversation

kevinmcconnell
Copy link
Collaborator

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
Collaborator Author

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
Collaborator Author

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
Collaborator Author

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
Collaborator Author

@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
Member

dhh commented Jul 29, 2022

Can you rebase?

@kevinmcconnell
Copy link
Collaborator Author

@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
Collaborator Author

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
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