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

Enable InstaClick by default #1162

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Enable InstaClick by default #1162

merged 6 commits into from
Feb 6, 2024

Conversation

afcapel
Copy link
Collaborator

@afcapel afcapel commented Feb 5, 2024

It will be the new default for Turbo 8. You can always opt out by setting <meta name="turbo-prefetch" content="false"> in the head of your HTML.

To help older apps with the transition we don't prefetch any links that contain the data-remote, data-behavior, data-method or data-confirm attributes. These are used by UJS in older apps to trigger unsafe behavior on links. More modern apps tend to implement these as <button>s instead.

//cc @dhh @davidalejandroaguilar

It will be the new default for Turbo 8. You can always opt out by setting
`<meta name="turbo-prefetch" content="false">` in the head of your HTML.
For compatibility with older apps that use UJS, we should not prefetch
links that have `data-remote`, `data-behavior`, `data-method`, or
`data-confirm` attributes.

All of these behaviors are now usually implemented as buttons, but
there are still some apps that use them on links.
Copy link
Collaborator

@kevinmcconnell kevinmcconnell left a comment

Choose a reason for hiding this comment

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

Love the idea of switching the default 👍 It's made such a noticeable difference in the places we've used it so far.

Just had a question about how we handle the UJS compatibility part, but it's your call.

Comment on lines 141 to 142
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.

To allow for more fine-grained control over when Turbo should prefetch
links. This change introduces a new event that can be used to cancel
prefetch requests based on custom logic.

For example, if you want to prevent Turbo from prefetching links that
include UJS attributes, you can do so by adding an event listener for
the `turbo:before-prefetch` event and calling `preventDefault` on the
event object when the link should not be prefetched.

```javascript
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")
}
```
Copy link
Contributor

@davidalejandroaguilar davidalejandroaguilar left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! 😺 , just have a non-blocking comment.

Excited this is going to be enabled by default on Turbo 8!

@afcapel afcapel merged commit cdf158f into main Feb 6, 2024
2 checks passed
@afcapel afcapel deleted the instaclick-by-default branch February 6, 2024 09:54
@navidemad
Copy link

is it possible to skip also the with data-turbo-stream="true" ?

@afcapel
Copy link
Collaborator Author

afcapel commented Feb 7, 2024

@navidemad InstantClick should work fine with data-turbo-stream links. Are you finding any issues?

If you don't want the feature for other reasons, you can always disable it with data-turbo-prefetch=false in a particular link or container, or globally with <meta name="turbo-prefetch" content="false">.

@kevinmcconnell
Copy link
Collaborator

@navidemad if you always want to skip any link that has data-turbo-stream="true" you could also do so in a turbo:before-prefetch event handler, along the line of this example that @afcapel posted earlier.

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.

None yet

5 participants