symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
816 stars 299 forks source link

[Turbo] InstantClick should not prefetch links from Web Debug Toolbar #1520

Open davidgorges opened 6 months ago

davidgorges commented 6 months ago

Turbo 8 comes with InstantClick by default. This prefetches links while the cursor is hovering, which is a nice feature.

The WebDebugToolbar intercepts Ajax calls, which include the prefetched requests via InstantClick and dynamically adds an entry to the Ajax menu. The makes it really hard to click on an entry.

https://github.com/symfony/ux/assets/3885619/33d95504-0c72-430b-aeba-c2a65ae118c4

I think it would nice be to disable the prefetching for the WebDebugToolbar via data-turbo-prefetch="false" directly in the Profiler Bundle, but since it's Turbo specific I am not sure if that's the right place.

Another way would be to add an event listener to the turbo:before-prefetch event that checks if the prefetch event comes from the WebDebugToolbar, e.g.

    document.addEventListener("turbo:before-prefetch", (event) => {
        if (event.target.closest('.sf-toolbar') !== null) {
            event.preventDefault()
        }
    })
WebMamba commented 6 months ago

Are you up to a PR? I think the event listener can be the right approach 😁

davidgorges commented 6 months ago

I can certainly try! I'd need some guidance, though. If we want to go with the event listener approach, do we want to inject this code only if the web debug toolbar js is injected?

That adds some complexity:

that seems a lot of code, I'm sure there's a simpler way.

WebMamba commented 6 months ago

All the links from the debug toolbar start with /_profiler maybe you can simply use that?

WebMamba commented 6 months ago

I just checked the web debug toolbar and it looks like there are already plenty of turbo attributes! So the solution of adding data-turbo-prefetch="false" can be a good solution as well. I think you should just try to find witch one is the most solid approach

smnandre commented 6 months ago

Turbo 8 comes with InstantClick by default. This prefetches links while the cursor is hovering, which is a nice feature.

For me this is a massive NO-GO for having this feature per default.

I'd suggest we disable it and only people wanting it enable it. That can mess with UI things, statistics ... and cost a lot of money in network data :|

smnandre commented 6 months ago

And thanks davidgorges for this issue and signaling this !

WebMamba commented 6 months ago

@smnandre why do you think it's a no-go to have it by default? Do you plan to create a PR on the Turbo repo to disable it by default ?

smnandre commented 6 months ago

Turbo is added per default now, i do believe this is a massive change (BC-type)

So we could either add the meta in the recipe, or better (to avoid polluting every project already using Turbo) add some js code to ignore it per default in the Turbo Bundle, yes :)

smnandre commented 6 months ago

As i said, pre-loading pages can mess with:

Not saying it's not a good feature, but it's imho a bit strong to force it everywhere for everyone :)

javiereguiluz commented 6 months ago

I agree with Simon. Enabling this by default seems dangerous, so I'd prefer if this option had to be enabled explicitly.

weaverryan commented 6 months ago

It was definitely an aggressive decision by the Turbo team. On the other hand, there is value in "being normal" - making our Turbo work just like the normal one. But I would feel ok either way.

arakneaweb commented 1 month ago

You can disable it by adding this meta tag to your page:

<meta name="turbo-prefetch" content="false">

https://turbo.hotwired.dev/handbook/drive#prefetching-links-on-hover

I strongly agree : Prefetch should not be activated by default. So much users move the mouse on the page as they are thinking and can hover over an element several times as they are wandering on a website or a webapp.