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

Allow Turbo Streams with GET via data-turbo-stream #612

Merged
merged 1 commit into from
Jul 14, 2022
Merged

Allow Turbo Streams with GET via data-turbo-stream #612

merged 1 commit into from
Jul 14, 2022

Conversation

kevinmcconnell
Copy link
Collaborator

Turbo Streams are normally supported only for non-GET requests. However there are cases where Turbo Streams responses to GET requests are useful.

This commit adds the ability to use Turbo Streams with specific GET requests by setting data-turbo-stream="true" on a form or link.

Turbo Streams are normally supported only for [non-GET requests][0].
However there are cases where Turbo Streams responses to GET requests
are useful.

This commit adds the ability to use Turbo Streams with specific GET
requests by setting `data-turbo-stream="true"` on a form or link.

[0]: #52
@kevinmcconnell
Copy link
Collaborator Author

If this change is accepted I'll also open a PR on https://github.com/hotwired/turbo-site to add it to https://turbo.hotwired.dev/reference/attributes.

@dhh
Copy link
Member

dhh commented Jun 30, 2022

Is the annotation required? What consequences would we have by simply always allowing streams in response to GET?

@kevinmcconnell
Copy link
Collaborator Author

kevinmcconnell commented Jun 30, 2022

Is the annotation required? What consequences would we have by simply always allowing streams in response to GET?

If there is existing code that relies on the current behaviour when choosing between stream responses and HTML responses for the same endpoint, then allowing streams in response to GET by default could be a breaking change.

Adding this behaviour via an annotation means it's easy for people to use streams with GET requests whenever they want to, but without risking any breaking changes. I think it's a pragmatic solution, given the existing behaviour has been in place for quite a while.

This style of using a data-turbo-* attribute to control behaviour is consistent with the existing customizations.


Note: edited the above to clarify the scope of this PR. There's some discussion about changing the default behaviour on #621. I think there's value in having that discussion, and we could consider whether it's something we should change in a future major version update. But, in hindsight, having that discussion here is probably distracting from the point of this PR, which is to make it easy to use stream responses with GET requests with minimal effort & risk.

@dhh dhh merged commit bdad71e into hotwired:main Jul 14, 2022
@dhh
Copy link
Member

dhh commented Jul 14, 2022

Need a PR to document this as well.

@kevinmcconnell
Copy link
Collaborator Author

Documentation is in hotwired/turbo-site#103

dhh added a commit to seanpdoyle/turbo that referenced this pull request Jul 15, 2022
* main:
  Drive Browser tests with `playwright` (hotwired#609)
  Allow Turbo Streams w/ GET via `data-turbo-stream` (hotwired#612)
  Only update history when Turbo visit is renderable (hotwired#601)
  Support development ChromeDriver version overrides (hotwired#606)
  Turbo stream source (hotwired#415)
  Expose Frame load state via `[complete]` attribute (hotwired#487)
dhh added a commit to feliperaul/turbo that referenced this pull request Jul 16, 2022
* main:
  Allow frames to scroll smoothly into view (hotwired#607)
  Export Type declarations for `turbo:` events (hotwired#452)
  Add .php as a valid isHTML extension (hotwired#629)
  Add original click event to 'turbo:click' details (hotwired#611)
  Drive Browser tests with `playwright` (hotwired#609)
  Allow Turbo Streams w/ GET via `data-turbo-stream` (hotwired#612)
  Only update history when Turbo visit is renderable (hotwired#601)
  Support development ChromeDriver version overrides (hotwired#606)
  Turbo stream source (hotwired#415)
  Expose Frame load state via `[complete]` attribute (hotwired#487)
  fix(ie/edge): form.method='delete', raises Invalid argument. (hotwired#586)
  Do not declare global types/constants (hotwired#524)
  Defensively create custom turbo elements (hotwired#483)
  Use `replaceChildren` in StreamActions.update (hotwired#534)
@seanabrahams
Copy link

seanabrahams commented Jul 18, 2022

@kevinmcconnell thank you for this. It was merged at an opportune time as I'm looking to implement a "Load More"-style of pagination using Turbo Streams via GET request.

On testing though I'm experiencing Turbo not including query params in data-turbo-stream GET requests generated from a link:

<div id="posts">
...
</div>
<a href="/posts?page=2" data-turbo-stream="true">
  Load More
</a>

Will end up sending only /posts to the backend with the page=2 param discarded.

I'll take a look to see if this is an easy fix but thought I'd pass it along in case the fix is obvious to you.

Currently experiencing this with f5a9dba.

Cheers.

Update: It looks like there have been significant changes recently that may be impacting this such as #631 and #630

@kevinmcconnell
Copy link
Collaborator Author

@seanabrahams oh good catch! I'm guessing this is because links with data-turbo-stream are being submitted as forms, and forms with method="GET" don't submit the query string part of their action. I expect the same thing would happen if you had a link with data-method="GET". We probably need to prevent the link from being converted into a form in this case.

I'll try to look into this today if I can. Thanks for letting me know about it.

@hey-leon
Copy link
Contributor

@kevinmcconnell this is awesome. Looking forward to using this 👀

@BakiVernes
Copy link
Contributor

Some issues I encountered

Not sure if this is the right place to post this but here we go. Decided to update my app and remove UJS altogether. Fetched the latest beta.1 release specifically to try this feature. Couple of things I noticed:

  • Works properly only if wrapped inside a turbo-frame element

Working

image

image


Not working

image

image


  • Fires request twice. Once with turbo_stream request format and once with html. And since there are no .html responds in my backend throws an error both on client and backend. Video below:
Screen.Recording.2022-07-27.at.13.05.30.mov

@kevinmcconnell
Copy link
Collaborator Author

Hi @BakiVernes. The requests firing twice is, I think, a separate issue that's being addressed in #645.

I've not been able to reproduce your first issue though. I'm using this outside of Turbo Frames here and it's working as expected for me. Could you share a code example that reproduces it?

@thibaudgg
Copy link

I tried this feature with the 7.2.0-beta.2 release and added the data-turbo-stream="true" attribute to my link. I got the proper TURBO_STREAM response as expected but it seems like the aria-busy="true" attribute in the html tag isn't removed like when clicking on any other Turbo enabled link. Is it expected? For us it's problematics as we are showing a spinner when aria-busy is true, which is now never going away.

@kevinmcconnell
Copy link
Collaborator Author

@thibaudgg I'll look into that (hopefully today) and will get back to you. Thanks for letting me know!

@kevinmcconnell
Copy link
Collaborator Author

@thibaudgg I've opened #681 to fix this. Hopefully that will fix the issue for you, but let me know if you have any questions or concerns.

Thanks again for reporting!

@thibaudgg
Copy link

@kevinmcconnell thanks a lot for the quick fix, I will give it a try next week. 👍🏻

@bilogic
Copy link

bilogic commented Sep 5, 2022

What should the default URL behavior be when using GET with data-turbo-stream?

Currently, it stays unchanged, I was expecting it to advance.

My use case is that I tag my navigation <a href=... with data-turbo-stream=true so that the server can return a turbo-stream to update another part of the HTML. URL needs to update as part of navigation.

@luctus
Copy link

luctus commented Oct 10, 2022

What should the default URL behavior be when using GET with data-turbo-stream?

Currently, it stays unchanged, I was expecting it to advance.

My use case is that I tag my navigation <a href=... with data-turbo-stream=true so that the server can return a turbo-stream to update another part of the HTML. URL needs to update as part of navigation.

Hey @bilogic, did you get any workaround here?

@bilogic
Copy link

bilogic commented Oct 10, 2022

@luctus no I did not

@kevinmcconnell
Copy link
Collaborator Author

What should the default URL behavior be when using GET with data-turbo-stream?

@bilogic the behaviour is to load the new content, but not trigger a visit, which matches what you're seeing. It's consistent with Turbo Stream responses over other HTTP methods (like POSTing a form). I think it's a good default, since a Turbo Stream response could be making any sort of change to the page, so we don't really know whether it represents a navigation or not. That also matches the default behaviour of navigations inside a <turbo_frame>, which are conceptually quite similar.

Perhaps we should add the ability to override this with data-turbo-action, similar to how that can be done with frames. That seems like it would be useful?

@luctus
Copy link

luctus commented Oct 12, 2022

Hello @kevinmcconnell,

Let's say we have a to-do list. In the index view we will have all the tasks listed, you click on one (with data-turbo-stream=true) and it triggers a slideover with the details of that task, so you can close it quickly and continue scrolling your list.

Once the slideover is open, I want to copy the url and share it with my friend, but the url will remain being the one for the index view.

Do you think this is not the right use case for this new feature? I may be super lost, tbh.

Thanks a lot for your work, anyway!

@bilogic
Copy link

bilogic commented Oct 12, 2022

https://discuss.hotwired.dev/t/simpler-way-to-think-about-drive-vs-frame-vs-stream/4406

Because hotwire is quite "new", I think this is a good way to think about our use cases.

@kevinmcconnell in your case, it seems to be click here, don't change here + change elsewhere (details), then copy and paste (which is not part of hotwire)

@pySilver
Copy link

@luctus in that case you should manage the URL state yourself. Here is how: #792

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.

9 participants