hotwired / turbo-rails

Use Turbo in your Ruby on Rails app
https://turbo.hotwired.dev
MIT License
2.09k stars 322 forks source link

Prefetch causes turbo stream to render on hover instead of on click #599

Closed BenCh94 closed 6 months ago

BenCh94 commented 6 months ago

We have a filtering page with several filter dropdowns, each dropdown contains a list of aggregations from our OpenSearch response and renders a link_to tag.

- agg["buckets"].each_with_index do |option, i|
   = link_to filter_datasets_path(option["key"].merge(current_filters))

these link_to tags sit inside a turbo_frame and the action responds with a turbo_stream that updates results and the filters on the page.

<%= turbo_stream.update("filters") do %>
  <%= render(partial: "filters", locals: { filters: @filters, results: @results}) %>
<% end %>
<%= turbo_stream.update("results") do %>
  <% @results[1]["hits"]["hits"].each do |dataset| %>
    <%= render(partial: "result", locals: { dataset: dataset }) %>
  <% end %>
<% end %>

In the latest release of turbo-rails these links are rendering the turbo stream response on hover before the user clicks anything. This seems to be a result of the pre-fetch links enabled by default in Turbo 8.

We're aware of the ability to disable the prefetch functionality on a per-element basis but is this the expected default behavior for a link_to that responds with a turbo_stream, should we be converting any link_to tags to button_to or some other tag to avoid this behavior in the future?

seanpdoyle commented 6 months ago

Thank you for opening this issue.

Are your <a> elements rendered with [data-turbo-stream]? If they are, they should be ignored by the Link Prefetcher.

If you're able, could you modify the script shared below to reproduce your behavior?

bug.rb ```ruby require "bundler/inline" gemfile(true) do source "https://rubygems.org" git_source(:github) { |repo| "https://github.com/#{repo}.git" } gem "rails" gem "propshaft" gem "sqlite3" gem "turbo-rails" gem "capybara" gem "cuprite", "~> 0.9", require: "capybara/cuprite" end require "active_record/railtie" # require "active_storage/engine" require "action_controller/railtie" require "action_view/railtie" # require "action_mailer/railtie" # require "active_job/railtie" # require "action_cable/engine" # require "action_mailbox/engine" # require "action_text/engine" require "rails/test_unit/railtie" class App < Rails::Application config.load_defaults Rails::VERSION::STRING.to_f config.root = __dir__ config.hosts << "example.org" config.eager_load = false config.session_store :cookie_store, key: "cookie_store_key" config.secret_key_base = "secret_key_base" config.consider_all_requests_local = true config.turbo.draw_routes = false Rails.logger = config.logger = Logger.new($stdout) routes.append do root to: "application#index" end end class ApplicationController < ActionController::Base include Rails.application.routes.url_helpers class_attribute :template, default: DATA.read def index render inline: template, formats: :html end end class ApplicationSystemTestCase < ActionDispatch::SystemTestCase driven_by :cuprite, using: :chrome, screen_size: [1400, 1400], options: { js_errors: true } end Capybara.configure do |config| config.server = :webrick config.default_normalize_ws = true end ENV["DATABASE_URL"] = "sqlite3::memory:" ENV["RAILS_ENV"] ||= "test" Rails.application.initialize! require "rails/test_help" class TurboSystemTest < ApplicationSystemTestCase test "reproduces bug" do visit root_path assert_text "Loaded with Turbo" end end __END__ <%= csrf_meta_tags %> Loaded without Turbo ```
BenCh94 commented 6 months ago

@seanpdoyle adding [data-turbo-stream] to the fixed this issue, thanks for the quick reply

radanskoric commented 4 months ago

I have found this issue because I ran into the same issue. An endpoint may respond with a turbo stream and the turbo stream was being executed on prefetching.

Adding data-turbo-stream indeed works but I am left wondering what is preventing Turbo from ignoring the prefetch result if it comes back as a turbo stream response to avoid having to remember to specify the attribute?

I am thinking that the following 2 statements are true based on the documentation and feature intent:

  1. Prefetching should never have any effect on the page until the click happens.
  2. Stream actions always have an effect on the page since that is their sole purpose.

Putting this to together it seems clear that prefetch should always ignore stream action Content-Type response.

Are there any reasons for not implementing this change to remove a non obvious prefetch pitfall?

radanskoric commented 4 months ago

I had a quick debug session to see why this is happening and the short answer is:

  1. StreamObserver#start listens for before-fetch-response and renders and stream messages into the body to get them to execute. It's doesn't discriminate and does this for all fetch requests. As far as I could tell, it's the only Turbo internal code listening to this event.
  2. LinkPrefetchObserver#tryToPrefetchRequest constructs a regular prefetch request but saves the response without acting on it. However, that happens after the callbacks fire, i.e. too late.

There's no way for the StreamObserver to differentiate fetches that are regular vs those originating from a prefetch.

The easiest way to fix it would be add a property for FetchRequest indicating if it is a prefetch request and then modify behaviour based on it. However that would couple StreamObserver and probably some other classes to the logic of prefetching, making it more expensive to maintain long term.

It would be better if handling of a stream response was decoupled from fetching, same as it is for regular HTML responses.

HTML response gets fetched and is later used by the other code. But a stream response fetching is intercepted and then the response is acted on during the fetching process.

This is fundamentally at opposition to the concept of prefetching. More importantly, it will also be a problem for any future feature that needs request fetching and response handling separated in any way.

I am not yet sure on how the refactoring should look, that would need to be discovered in code. I am interested to try that but before putting effort into it, I'd like to know if this work would be even welcomed by the maintainers?