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

Enable InstaClick by default #1162

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Enable InstaClick by default #1162

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
09d9fc4
Enable InstaClick by default
afcapel Feb 5, 2024
127fc18
Don't prefetch any anchor links
afcapel Feb 5, 2024
1251953
Don't prefetch UJS links
afcapel Feb 5, 2024
5b866db
Allow to customize check to opt out of prefetched links
afcapel Feb 5, 2024
bca14e4
Tweak documentation
afcapel Feb 5, 2024
7327a95
Introduce `turbo:before-prefetch` event
afcapel Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/observers/link_prefetch_observer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export class LinkPrefetchObserver {
}

#tryToPrefetchRequest = (event) => {
if (getMetaContent("turbo-prefetch") !== "true") return
if (getMetaContent("turbo-prefetch") === "false") return

const target = event.target
const isLink = target.matches && target.matches("a[href]:not([target^=_]):not([download])")
Expand Down Expand Up @@ -134,7 +134,11 @@ export class LinkPrefetchObserver {
#isPrefetchable(link) {
const href = link.getAttribute("href")

if (!href || href === "#" || link.getAttribute("data-turbo") === "false" || link.getAttribute("data-turbo-prefetch") === "false") {
if (!href || href.startsWith("#") || link.getAttribute("data-turbo") === "false" || link.getAttribute("data-turbo-prefetch") === "false") {
return false
}

if (link.hasAttribute("data-remote") || link.hasAttribute("data-behavior") || link.hasAttribute("data-method") || link.hasAttribute("data-confirm")) {
return false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to not build these checks in, but instead document the new behaviour, and have a cancellable event people can use to avoid prefetching according to their own needs. I can definitely see how this is helpful for people with older codebases, but it also feels like Turbo is having to carry knowledge about UJS, and once we have it, it's harder to remove later.

I don't feel very strongly about it, though. There's pros and cons either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if we're keeping it, maybe worth extracting this to give it a helpful name, like #hasUJSattributes or similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinmcconnell it is a trade-off, for sure, but I think in this case the pros outweigh the cons. Having this in place means that older applications can upgrade without needing any custom code. Without this the upgrade for an application like Basecamp was far from trivial. And adding a cancellable event as an extension point is also extending the public API in a way we'll have to maintain.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make this as "drop-in replacement" as possible. Even for older apps. But I'm wondering if we can extract all those checks into a single function that just happens to be the default before filter on this. A filter that you can then overwrite with your own function, if you either need more/less/different on the opt-out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an extension point so now you can do something like:

Turbo.preventLinkPrefetch((link) => link.hasAttribute("hx-post"))

But I find a bit confusing that it only overrides some checks, not all of them. We have several different conditions, checking things such as whether the link has the same origin, if it has data-turbo=false, if it targets an iframe, etc. I don't think there's a practical use case to remove any of them, the prefetch is not going to work in those cases.

We could allow you to redefine the whole logic, including the defaults, but then you'll have to recreate all these checks in application logic, which may be error prone and also you'll miss out if the framework updates this behavior.

I think the most practical use case is adding more checks on top of the defaults. But in that case I think a cancellable event, as @kevinmcconnell suggested, makes more sense and feels more idiomatic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on that. Only that this should be on by default, and we should account for these common caveats with earlier Rails apps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks. I've added a cancellable event in 7327a95. This can be used add custom logic to opt out of prefetching.

document.body.addEventListener("turbo:before-prefetch", (event) => {
  if (isUJSLink(event.target)) event.preventDefault()
})

function isUJSLink(link) {
  return link.hasAttribute("data-remote") || link.hasAttribute("data-behavior") || link.hasAttribute("data-method") || link.hasAttribute("data-confirm")
}

We'll add this to the documentation so older apps have an easy migration path.

}

Expand Down
2 changes: 2 additions & 0 deletions src/tests/fixtures/hover_to_prefetch.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
>Won't prefetch when hovering me</a>
<a href="/src/tests/fixtures/prefetched.html" id="anchor_with_turbo_false" data-turbo="false"
>Won't prefetch when hovering me</a>
<a href="/src/tests/fixtures/prefetched.html" id="anchor_with_remote_true" data-remote="true"
>Won't prefetch when hovering me</a>
<a href="/src/tests/fixtures/hover_to_prefetch.html" id="anchor_for_same_location"
>Won't prefetch when hovering me</a>
<a href="/src/tests/fixtures/prefetched.html?foo=bar" id="anchor_for_same_location_with_query"
Expand Down
5 changes: 5 additions & 0 deletions src/tests/functional/link_prefetch_observer_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ test("it doesn't prefetch the page when link has data-turbo=false", async ({ pag
await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_turbo_false" })
})

test("it doesn't prefetch the page when link has data-remote=true", async ({ page }) => {
await goTo({ page, path: "/hover_to_prefetch.html" })
await assertNotPrefetchedOnHover({ page, selector: "#anchor_with_remote_true" })
})

afcapel marked this conversation as resolved.
Show resolved Hide resolved
test("it doesn't prefetch the page when link has the same location", async ({ page }) => {
await goTo({ page, path: "/hover_to_prefetch.html" })
await assertNotPrefetchedOnHover({ page, selector: "#anchor_for_same_location" })
Expand Down
Loading