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

Turbo-frame caching doesn't seem to work when preloading frame links. #857

Closed
bennadel opened this issue Jan 30, 2023 · 9 comments · Fixed by #1033
Closed

Turbo-frame caching doesn't seem to work when preloading frame links. #857

bennadel opened this issue Jan 30, 2023 · 9 comments · Fixed by #1033

Comments

@bennadel
Copy link

I raised this issue on the dev forum, and it was suggested that I open an Issue here - apologies if this is not the correct course of action.

I am attempting to use a <turbo-frame id="..." data-turbo-action="advance"> in conjunction with some embedded links that preload the content, <a href="..." data-turbo-preload>.

Here's what I do see:

  1. The turbo-frame works in so much as the embedded links make a request (when clicked) and re-render the frame content only (leaving the rest of the page untouched).
  2. Turbo drive pre-fetches the <a> tag content immediately when the page loads.

What I don't see:

  1. A Preview of the cached link rendered immediately upon clicking (as fetch is made in the background).
  2. Two turbo:render events (documentation says one should fire for the preview and then another for the fresh content).

Here's a truncated version of the code that I am running - it's intended to be a "slide show" kind of thing for quotes:

<turbo-frame id="quote-frame" data-turbo-action="advance">
	<figure>
		#encodeForHtml( quote.excerpt )#
	</figure>
	<div>
		<a
			href="index.htm?quoteIndex=#encodeForUrl( quote.prevIndex )#"
			data-turbo-preload>
			Prev quote
		</a>
		<a
			href="index.htm?quoteIndex=#encodeForUrl( quote.nextIndex )#"
			data-turbo-preload>
			Next quote
		</a>
	</div>
</turbo-frame>

Over on the dev forum, someone indicated that caching for turbo-frame links works ... but, not in conjunction with data-turbo-preload. Thanks for any help you can offer!
@seanpdoyle
Copy link
Contributor

Thank you for opening this issue!

At the moment, pre-loading <a> elements doesn't incorporate any contextual Turbo Frame information:

turbo/src/core/drive/preloader.ts

Lines 37 to 53 in f04a21c

async preloadURL(link: HTMLAnchorElement) {
const location = new URL(link.href)
if (this.snapshotCache.has(location)) {
return
}
try {
const response = await fetch(location.toString(), { headers: { "VND.PREFETCH": "true", Accept: "text/html" } })
const responseText = await response.text()
const snapshot = PageSnapshot.fromHTMLString(responseText)
this.snapshotCache.put(location, snapshot)
} catch (_) {
// If we cannot preload that is ok!
}
}

What's crucially missing here is the Turbo-Frame header.

If we were to support that, the Preloader would need to evaluate which <turbo-frame> (if any) the <a> element would drive:

  • does it have a <turbo-frame> ancestor
  • does it reference a <turbo-frame> through its [data-turbo-frame] attribute
  • does the <turbo-frame> element referenced by either of the above bullets declare [target] to reference another <turbo-frame> element elsewhere
  • continue walking that dependency graph until it knows for sure which element it'd drive

At any point in that traversal, the Preloader would need to check for the [disabled] attribute.

That logic exists throughout Turbo, as it's what powers Turbo Frame navigation. With that being said, there isn't a centralized location or method for the Preloader to call.

Even if that were available, I'm not sure of the implications of writing the response HTML to the PageSnapshot cache, since there's a precedent for response HTML for requests with Turbo-Frame to omit their page layout HTML (including <html>, <head>, and <body> elements), and the PageSnapshot cache expects full HTML documents.

@bennadel
Copy link
Author

bennadel commented Jan 30, 2023 •
edited
Loading

@seanpdoyle thank you for the detailed reflection on the state of things. I'm on like "Day 2" of learning Hotwire, so I'm still very much finding my sea legs for how this all fits together. Your point about frames responses often omitting much of the layout showcases the complexity of this stuff.

Just spit-balling here, but perhaps the data-turbo-preload attribute could be an enumeration rather than a boolean attribute, where if a value is provided, such as:

<turbo-frame id="my-frame">
    <a href="..." data-turbo-preload="my-frame">Do the thing</a>
</turbo-frame>

... it could point to a frame-specific cache (something like FrameSnapshot).

Anyway, just thinking out load with basically no historical context 😆 I'm just excited to start learning more about this stuff.

@bennadel
Copy link
Author

I just wanted to share a write-up that I did of the task I was trying to accomplish: A Simple Slide Show Using Hotwire And Lucee CFML . Obviously, it doesn't have the preload functionality; but, it better illustrates what I am trying to accomplish).

@seanpdoyle seanpdoyle mentioned this issue Oct 12, 2023
Merged
@seanpdoyle
Copy link
Contributor

@bennadel I've opened #1033. Does that cover what's described here?

@bennadel
Copy link
Author

@seanpdoyle It sounds good, though the code is a bit over my head. It's been a while since I've thought about this stuff. I've been focusing on other things, but will get back to Hotwire soon.

@seanpdoyle
Copy link
Contributor

@bennadel thank you! A portion of that diff is related to housekeeping, and involves a lot of implementation details.

The clauses that happen to exclude Turbo Frames are in this portion: https://github.com/hotwired/turbo/pull/1033/files#diff-1a0b7d769d04149117769a2bb8e5b27af85123be3c37e31f5790113c827a1962R132-R147

@bennadel
Copy link
Author

@seanpdoyle Hmmm, that makes it look it won't preload if the link is inside a tubro-frame (frame instanceof FrameElement)?

@pySilver pySilver mentioned this issue Dec 4, 2023
Open
@pySilver
Copy link

pySilver commented Dec 4, 2023

@bennadel Did you manage to solve your slideshow cache issue somehow? I'm having a similar problem with my navigation menu containing dynamically loaded sub-menus. The problem is that every time the user navigates to the sub-menu he has to wait for the server response which makes the menu slow. Looks like frame navigation is not cached.

@seanpdoyle
Copy link
Contributor

This was closed by #1033. Prior to 1033, support for Turbo Frames could be expected, but based on #857 (comment), I think it'll require some more consideration before committing to that support.

#1033 incorporates guards to reject Turbo Frames. I think pre-loading frames is viable, and worth exploring separately.

@afcapel if we think that's important for v8, we could re-open this issue.

@seanpdoyle seanpdoyle mentioned this issue Dec 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Guard [data-turbo-preload] with conditionals seanpdoyle/turbo
3 participants
@bennadel @pySilver @seanpdoyle