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

Turbo doesn't send Turbo-Frame header when frame's src is updated #1191

Open
steerio opened this issue Feb 19, 2024 · 14 comments · May be fixed by #1198
Open

Turbo doesn't send Turbo-Frame header when frame's src is updated #1191

steerio opened this issue Feb 19, 2024 · 14 comments · May be fixed by #1198

Comments

@steerio
Copy link

steerio commented Feb 19, 2024

According to the documentation, when a request is made to update a Turbo Frame, the Turbo-Frame HTTP header should be set to the ID of that frame.

This is not happening in Turbo 8.0.2 (delivered with Turbo-Rails 2.0.3).

To reproduce:

  • Get hold of a Turbo Frame that has an ID (in a Stimulus controller or in the Javascript console).
  • Issue frame.src = "/some-url".
  • Observe that the resulting request has no Turbo-Frame header.
@seanpdoyle
Copy link
Contributor

seanpdoyle commented Feb 19, 2024

Thank you for opening this issue. I've tried to reproduce it on main with the following change:

index f192590..c31bfc0 100644
--- a/src/tests/functional/frame_tests.js
+++ b/src/tests/functional/frame_tests.js
@@ -2,6 +2,7 @@ import { test, expect } from "@playwright/test"
 import { assert, Assertion } from "chai"
 import {
   attributeForSelector,
+  getSearchParam,
   hasSelector,
   listenForEventOnTarget,
   nextAttributeMutationNamed,
@@ -82,6 +83,21 @@ test("following a link sets the frame element's [src]", async ({ page }) => {
   assert.equal(src.searchParams.get("key"), "value", "[src] attribute encodes query parameters")
 })
 
+test("setting the element's [src] navigates the frame", async ({ page }) => {
+  const frame = page.locator("#frame")
+
+  await expect(frame.locator("h2")).toHaveText("Frames: #frame")
+  await frame.evaluate((frame) => frame.src = "/src/tests/fixtures/frames/frame.html?key=value")
+
+  const { url, fetchOptions: { headers } } = await nextEventOnTarget(page, "frame", "turbo:before-fetch-request")
+
+  expect(headers["Turbo-Frame"]).toEqual("frame")
+  expect(pathname(url)).toEqual("/src/tests/fixtures/frames/frame.html")
+  expect(getSearchParam(url, "key"), "fetch request encodes query parameters").toEqual("value")
+
+  await expect(frame.locator("h2")).toHaveText("Frame: Loaded")
+})
+
 test("following a link doesn't set the frame element's [src] if the link has [data-turbo-stream]", async ({ page }) => {
   await page.goto("/src/tests/fixtures/form.html")

Unfortunately, I was not able to fail the test. Does it reflect the circumstances in which you've encountered this unexpected behavior?

In case you want to try it locally, here's the block:

test("setting the element's [src] navigates the frame", async ({ page }) => {
  const frame = page.locator("#frame")

  await expect(frame.locator("h2")).toHaveText("Frames: #frame")
  await frame.evaluate((frame) => frame.src = "/src/tests/fixtures/frames/frame.html?key=value")

  const { url, fetchOptions: { headers } } = await nextEventOnTarget(page, "frame", "turbo:before-fetch-request")

  expect(headers["Turbo-Frame"]).toEqual("frame")
  expect(pathname(url)).toEqual("/src/tests/fixtures/frames/frame.html")
  expect(getSearchParam(url, "key"), "fetch request encodes query parameters").toEqual("value")

  await expect(frame.locator("h2")).toHaveText("Frame: Loaded")
})

@steerio
Copy link
Author

steerio commented Feb 19, 2024

Thanks, Sean, I'll check that.

In the meantime I created this minimal Rails app to showcase the issue.

@seanpdoyle
Copy link
Contributor

@steerio thank you for creating that repository!

As a piece of advice, git diffs can be really useful in reproduction repositories. In its current format, that repository has a single commit that includes both rails new-generated content alongside the specific changes you've made to demonstrate the issue. It's almost impossible to separate the two types of code at a glance without reading every Ruby, JavaScript, or HTML file and mentally comparing them to what I expect a new Rails application to have.

Could you revise the repository so that the "Intial commit" only contains the rails new code, then a second commit only contains the changes necessary to reproduce this behavior?

@steerio
Copy link
Author

steerio commented Feb 19, 2024

All right, I've done this and made a force push to the same repo, and here is the commit with the changes.

To see it in action you only need to bundle install and then rails s. Everything is on the front page.

It's worthy to note that if you downgrade turbo-rails with this change in the Gemfile, it all starts working.

-gem "turbo-rails"
+gem "turbo-rails", "<2"

@steerio
Copy link
Author

steerio commented Feb 19, 2024

I noticed something interesting with the demo code:

  1. The page is already loaded on mouseover. Looks like this is InstaClick, which indeed has been introduced by v8.
  2. The first click makes no request, hinting that the preload is being reused.
  3. If you make two clicks (while managing to stay over the link), the second one will work properly.

So the problem is that the InstaClick preload is used even when the final request is not exactly the same as the preload one. In our case, when the request is preloaded, Turbo expects a visit to happen (and can't have any idea about our onclick intentions), so that request is made without the header we're expecting to be present.

The second click (provided that you didn't leave the link with the mouse before it) will make a proper request.

I think the situation can be rectified by adding the headers to the cache key. For the time being, I'll switch it off with a meta tag - even with that fix, we'd have two requests done instead of one.

@steerio
Copy link
Author

steerio commented Feb 19, 2024

To add a bit of an explanation: our use case is that some pages can be loaded in a modal and standalone as well. To tell these situations apart (and to give an ID to the Turbo Frame holding the content) we're using the Turbo-Frame header.

We also try to cut down on payload sizes for larger, Turbo-heavy pages, so in case of Turbo Frame requests, we try to only render what was requested.

We defined some handy helpers to help with this kind of thing:

  • requested_frame: string, the value of the header basically.
  • frame_request?: boolean, whether the header was present.
  • frame_wanted?(id): boolean, true if the header equals to the argument or if the header was missing

It all worked well until turbo-rails 2 bumped the version of Turbo to v8.

@seanpdoyle
Copy link
Contributor

I think you're correct that pre-fetching is at the root of this behavior.

In the meantime, are you able to render <meta name="turbo-prefetch" content="false"> into your <head> element?

@steerio
Copy link
Author

steerio commented Feb 19, 2024

Yes, that's what I ended up doing, and it works.

@seanpdoyle
Copy link
Contributor

Looping in @afcapel @davidalejandroaguilar to help troubleshoot this behavior.

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Feb 21, 2024
Closes [hotwired#1191][]

Ensure Prefetching does not share cache entries for requests with
differing `Turbo-Frame:` HTTP headers.

[hotwired#1191]: hotwired#1191
@seanpdoyle seanpdoyle linked a pull request Feb 21, 2024 that will close this issue
@seanpdoyle
Copy link
Contributor

@steerio I've opened #1198 to try and resolve this issue. I've added tests that aim to reproduce the issue as you've described it.

Unfortunately, they pass without any implementation changes. Could you provide some feedback on that PR to make those tests fail consistently?

@steerio
Copy link
Author

steerio commented Feb 23, 2024

@seanpdoyle I don't readily have a system that would be able to run these tests (Arch has some issues with libpcre.so.3 and they didn't work on a Mac for some reason either). I'll set up a Debian in Docker, have them all pass, and see if I can write one that fails on this issue.

Just by glancing over it I'd say that it probably works with frame requests that it triggers itself, i.e. even at mouseover time it knows that when clicked, it would load in a given frame. In my case there was a Stimulus controller (which could be any event listener) on the click event with custom code that would do that.

This custom code simply updates the src attribute on a Turbo frame, and this should not blindly reuse the mouseover cache. Also, there could be a way to disable the prefetch feature on a per-link basis, because if this bug is fixed, there's still a useless extra request creating unnecessary load.

@seanpdoyle
Copy link
Contributor

there could be a way to disable the prefetch feature on a per-link basis

Have you tried adding the [data-turbo-prefetch="false"] attribute to the link?

@steerio
Copy link
Author

steerio commented Feb 23, 2024

I haven't - I disabled the whole thing (which is fairly new to me, having only recently updated) via the meta tag. This looks like a nice alternative, thanks for pointing it out.

@texpert
Copy link

texpert commented Sep 22, 2024

there could be a way to disable the prefetch feature on a per-link basis

Have you tried adding the [data-turbo-prefetch="false"] attribute to the link?

@seanpdoyle , I have been trying with data-turbo-prefetch="false", because when prefetch is enabled, there are 2 requests - the 1st is OK, but the click is firing another request instead of using the prefetched data, and the 2nd request is a full page load. So, I've tried to disable prefetching just to find that no Turbo-Frame header is set.

I went through debugging it in the browser and found that it is going through Visit, whose prepareRequest method doesn't set the Turbo-Frame header.

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

Successfully merging a pull request may close this issue.

3 participants