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

turbolinks "instantclick" #313

Open
Enalmada opened this issue Aug 9, 2017 · 53 comments
Open

turbolinks "instantclick" #313

Enalmada opened this issue Aug 9, 2017 · 53 comments
Labels

Comments

@Enalmada
Copy link

@Enalmada Enalmada commented Aug 9, 2017

I want to use turbolinks but there is something it isn't doing that others seem to be doing...fetching content on hover and using that on click.

This is the main idea behind instantclick.io and the concept seems critical to unlocking the full performance potential of turbolinks. Note that there seems to be some past discussion about prefetching in general that went to dark places (#84) talking about plugins and not universally supported hints. I feel like hints and plugins are overboard...I feel like doing the same thing as instaclick deserves to be natively supported...even the default behavior (with an opt-out attribute for server side analytics or mutable links).

If turbolinks did a fetch of link on hover (rather than click), and used that content on click, it would reduce up to several hundred milliseconds of latency for the average user. It is hard to believe at first but it is true...try it for yourself: http://instantclick.io/click-test. For a properly tuned backend just dealing with network latency, that amount of time can be the difference in user perception between a site being fast and being instant. (Note that barbajs also has similar option)

So turbolinks, can you please consider doing this?

@joeldrapper

This comment has been minimized.

Copy link

@joeldrapper joeldrapper commented Aug 16, 2017

I really like this idea! InstantClick feels very fast compared with Turbolinks, especially on a slow 3G connection.

instant_click

In some cases, I think it would make sense to load the page into the cache on hover, but still do a normal request on click, in case the content has changed since hovering. Perhaps there should be an option for this?

It would also need to be specifically limited to GET requests, as prefetching POST, PATCH, and DELETE links could have unintended consequences.

@sstephenson what are your thoughts?

@Enalmada

This comment has been minimized.

Copy link
Author

@Enalmada Enalmada commented Aug 16, 2017

If anyone wants a tease at the potential of this, the gist from glennfu works as a drop in example: https://gist.github.com/Enalmada/6e11191c1ea81a75a4f266e147569096. If you were going to use in production you would probably want to modify to not blow out the normal turbolinks cache after lots of hover (to avoid blowing out your backspace position). In the production version of this concept, I believe by default only the latest hover should be cached (which could be a quick fix to the cache blowout) and hover cache should be cleared after click but all this could be configurable depending on how static a site is.

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Oct 23, 2017

Would it make sense to roll somethin in like @Enalmada's example behind a flag? Then have users either run Turbolinks.enable_insta_click(), or perhaps tags links with <a href='#' data-tubrolinks-insta-click>.

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Oct 24, 2017

I decided to try this out and put together this gist based on the one by @Enalmada . I added a check for data-turbolinks-preload="true" on the link, and it works pretty well. With the added check you get regular turbolinks for all links, and prefetch on selected links.

I still need to look into the cache blowout issue.

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Nov 16, 2017

+1 on this, would love to push @Ch4s3 fix to production but am concerned about cache blowout. Is it an idea to clear the cache after x amount of clicks, e.g: call Turbolinks.clearCache()?

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Nov 16, 2017

@panoply I think Turbolinks. clearCache() after x clicks is possibly correct, but I'd want other folks to weigh in. Also, my gist is in es6 and not coffeescript as I don't really know coffeescript, so we'd need to caffeinate the code. I'd be happy to work on this, but I'd need some input on coffeescript and the codebase.

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Nov 16, 2017

@Ch4s3 I fall into the same boat, coffeescript is yet to become apart of my knowledge stack. In theory one could easily log x click in conjunction with LocalStorage state. It's adding that extra layer but is how I would initially tackle it, going further, one could even prevent the preload running if page already exists in cache (i'm not sure if this is something already included by default). Your current edition increased page-to-page visits by 200ms – 300ms for me.

@Enalmada

This comment has been minimized.

Copy link
Author

@Enalmada Enalmada commented Nov 16, 2017

I believe that a low risk high reward default could be precaching only the current hover (re-caching that hover if user moves away and hovers over it again). Average users would feel the extreme "instaclick" performance sensation with negligible risk of stale content. Sites that have mostly mutable links could easily disable it, static content (ex: blog) sites could increase cache size or allow cache to span multiple pages.

precache-size: 1 (set to 0 to disable feature). Note: value of 1 fetches fresh content with every hover for balance between latency and risk of stale content. Increase cache size to 2+ to enable cache to persist beyond hover.)
clear-cache-on-click: true (to ensure min risk of stale content by default. ie: user hovers a link, posts a comment to site, clicks a link expecting to see their comment)
To disable preload on a per-link basis, add this: data-turbolinks-preload="false"

(I mean...the gists floating around ALMOST work...the final version just needs make sure precache wont ever blow out normal cache so users don't lose back button page position if they go hover crazy. I assume it might be best to implement with a completely separate precache storage instance that is checked first rather than shared.)

Bottom line...could someone with turbolinks experience help take this from gist to something more official? I don't know turbolinks or coffee but after experiencing how amazing "instaclick" makes a site feel with just around 100 lines of code, this is too important to let languish.

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Nov 17, 2017

@panoply

Your current edition increased page-to-page visits by 200ms – 300ms for me.

That sounds about like the latency of the server round trip, so its working as expected 👍

@Enalmada yeah, I'd be in favor of something low risk, and the more I think about it, only precaching 1 link at a time would be good.

Sites that have mostly mutable links could easily disable it

I'd probably prefer to have this be a per link opt in, like my gist. that makes it super narrow, which I like, but you could add it programmatically too.

could someone with turbolinks experience help take this from gist to something more official

As stated previously, I'd be happy to help, but I need some direction.

@Enalmada

This comment has been minimized.

Copy link
Author

@Enalmada Enalmada commented Nov 17, 2017

I guess whether you prefer opt-in or opt-out totally depends on what percentage of your site links have side-effects (or server analytics). I have no doubt some flags could be created to flip it one way or the other to make everyone happy. I wonder what is the best for the average user though. If I had to guess I would say the average link doesn't have side effects...so keep code DRY and follow the turbolinks pattern.

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Nov 17, 2017

@Enalmada I agree. Let's take a webshop for example, a user browsing the "products" page are going to be hovering over each product image within a grid and without an opt-in / opt-out this would make for a lot of unnecessary requests. In the use-case scenario an opt-in would be something one would most definitely require. It may also be an idea to possibly integrate a setTimeout() before the prefetch initializes, for example if users mouse is hovering ahref over 1500ms, then begin the prefetch.

Really interested in exploring this further.

@Enalmada

This comment has been minimized.

Copy link
Author

@Enalmada Enalmada commented Nov 17, 2017

@panoply That totally makes sense and it will probably be common for people to have special sections of the site they want to preload with care or not at all. The important point here is that hopefully choice can be up to them without making implementation/documentation any more complicated for the majority case.

I am just thinking aloud here but perhaps the right thing to do is not define the touchstart/mouseover event handlers in the plugin but allow people to define their own.

$("a:not(.opt-out)").addEventListener("mouseover", preload);
$("a.opt-in").addEventListener("mouseover", preload);

Perhaps for the special cases, having an example of something like this could be interesting: jquery-hoverIntent.

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Mar 1, 2018

@Enalmada and @panoply have you two given any more thought to this?

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Mar 1, 2018

@Ch4s3 not currently. I'd like to see this feature integrated but I am not prepared to learn or get familiar with coffeescript, especially with the advances in ES7 and ES8. I can confirm the methods I have tried in conjunction with Turbolinks increased performance, even the click to action time ratio was increased with preloading on hover, but adding an extra layer on top of Turbolinks is always a mission in itself, the maintainers are super picky.

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Mar 1, 2018

@panoply Yeah I'm aware of their general selectiveness. I wonder if it makes sense then to release this as a plugin of sorts. Maybe an npm package and or gem that adds this as a layer on top. That way the maintainers here don't have to take it on in the short run, and we could roll it in ES7.

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Mar 2, 2018

@Ch4s3 I would be far more inclined to see this under a plugin as sorts. If we take that route we could introduce something to the prototype property of Turbolinks, because in my head it would make more make more sense to do something like: Turbolinks.preloadClick(boolean) to initialize the plugin, right after Turbolinks.start() The question remains though how it would be best implemented. Just off the top of my head, a plugin like this would only really be necessary for desktop devices, Turbolinks wouldn't need this loading in on mobile and touch devices. The real gains would be those sites that are "image" heavy, that's where I find that delay and lag comes in on Turbolinks.

I'm interested nonetheless, would be happy to help.

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Mar 2, 2018

@panoply I'll read up on best practices for starting new npm packages and ping you once I have something going. I'll start working here over the weekend I think.

@jpic

This comment has been minimized.

Copy link

@jpic jpic commented May 14, 2018

I thought we already had that in Turbolinks 😂

@Enalmada

This comment has been minimized.

Copy link
Author

@Enalmada Enalmada commented May 24, 2018

FYI Until instantclick behavior is turbolinks native (or plugin) ready, you could consider doing a prefetch of any link a user stops moving over:

// https://briancherne.github.io/jquery-hoverIntent/
$(".prefetch").hoverIntent(prefetch);

function prefetch() {
        var link = $(this).attr("href");
        if (typeof link === typeof undefined || link === false) {
            link = $(this).attr("data-href");
        }
        var prerenderLink = $("#prefetchLink");
        if (prerenderLink.length) {
            if (prerenderLink.attr("href") === link) return;
            prerenderLink.attr("href", link);
        } else {
            $('<link id="prefetchLink" rel="prefetch" href="' + link + '" />').appendTo("body");
        }
}

(Inspired by this question)

Note that pages must be browser cacheable to have benefit so dynamic user content pages will need to be something like Cache-Control: private, max-age=5.

Todo: hoverintent adds some delay so make it optional by only using it if loaded or opt out class (.prefetch-immediate), offer opt in/out by letting people submit selector (ie a:not[.no-prefetch]), ignore invalid link like hash or javascript placeholder, offer rel=prerender opt in for people who understand the consequences.

Perhaps just making this more robust and into a plugin could be the best thing to do for now for the turbolinks community.

@grosser

This comment has been minimized.

Copy link

@grosser grosser commented Jun 8, 2018

Cleanup up version of above:

  • removing the data-href because idk what it does
  • docs
  • only prefetch paths and not mailto or outside urls
  • also prefetch new links after turbolinks transition
  • apply to all turbolinks links
// See https://github.com/turbolinks/turbolinks/issues/313
// Using https://briancherne.github.io/jquery-hoverIntent/
// and turbolinks prefetch to make clicks instant
// on page-load and after turbolinks transition
$(document).on("turbolinks:load", function(){
  var $prefetcher;

  $("a[data-turbolinks!=false]").hoverIntent(function(){
    var href = $(this).attr("href");
    if(!href.match(/^\//)){ return; } // do not prefetch outside urls or mailto:

    // add or change the prefetched link, be careful not to preload the same href multiple times
    if ($prefetcher) {
      if($prefetcher.attr("href") != href) {
        $prefetcher.attr("href", href);
      }
    } else {
      // NOTE: pre-creating the link does not work
      $prefetcher = $('<link rel="prefetch" href="' + href + '" />').appendTo("body");
    }
  });
});

Rails:

# Gemfile
gem 'rails-assets-jquery-hoverintent', source: 'https://rails-assets.org'

took our site from ~140 mouse-down-to-paint to ~40ms 🎉 (rest is mostly paint + js overhead)

screen shot 2018-06-08 at 9 34 21 am
screen shot 2018-06-08 at 9 35 12 am

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Jun 12, 2018

Eh, jQuery 🙄

@danielcompton

This comment has been minimized.

Copy link

@danielcompton danielcompton commented Jun 13, 2018

There is also https://github.com/tristen/hoverintent which is a port of jQuery-hoverintent.

@Ch4s3

This comment has been minimized.

Copy link

@Ch4s3 Ch4s3 commented Jun 20, 2018

I still need to dig into this as a separate project, it's on my TODO list.

@hopsoft

This comment has been minimized.

Copy link

@hopsoft hopsoft commented Jul 23, 2018

Here's my stab at this. It has no dependencies. I'd love some extra eyeballs to verify that it works properly. https://gist.github.com/hopsoft/ab500a3b584e2878c83137cb539abb32

Some details on the gist.

  • Anchors must explicitly opt-in to prefetch behavior. <a data-prefetch='true' ...>
  • The hover delay is hard coded at 600ms
@jpic

This comment has been minimized.

Copy link

@jpic jpic commented Sep 2, 2018

@hopsoft thanks, i thought it was working for me, but then started to find this kind of errors

[Learn More] index.js:94:8
../../../src/crudlfap/js/index.js/</fetchers[url]</<
index.js:94:8
[Show/hide message details.] TypeError: window.Turbolinks.Snapshot.fromElement is not a function[Learn More] index.js:94:8
Will-change memory consumption is too high. Budget limit is the document surface area multiplied by 3 (702832 px). Occurrences of will-change over the budget will be ignored. mrsrequest
[Show/hide message details.] TypeError: window.Turbolinks.Snapshot.fromElement is not a function[Learn More] index.js:94:8
@jpic

This comment has been minimized.

Copy link

@jpic jpic commented Sep 2, 2018

My bad, this method has been renamed to from fromHTMLElement 84303b9

@hopsoft

This comment has been minimized.

Copy link

@hopsoft hopsoft commented Sep 3, 2018

@jpic Thanks. I updated the gist.

@jpic

This comment has been minimized.

Copy link

@jpic jpic commented Sep 3, 2018

@hopsoft thank you for that 😂

I got it to work, but changed it to my needs

For me this still downloads the page when the user clicks, even if it already downloaded it during hover prefetch. I suppose this just means that turbolinks uses cache for preview and then refresh when it gets the page ? That's not blocking but could be avoided maybe ? Or am I getting this wrong ?

Thanks in advance for sharing some of your insight, I love this feature 😂

@hopsoft

This comment has been minimized.

Copy link

@hopsoft hopsoft commented Sep 5, 2018

Ah interesting. Yeah the preview makes it feel faster to the user but we could likely avoid the second request altogether. Need to think through how to best accomplish that. The prefetch will warm any Rails caching you have though... making the second request faster.

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Sep 9, 2018

Stumbled across this solution recently regarding InstantClick functionality. It goes without saying, I've been using @Enalmada solution in production with some minor edits and it's been working fine. We actually increased conversion rates after implementing.

@talhashoaib

This comment has been minimized.

Copy link

@talhashoaib talhashoaib commented Sep 9, 2018

@panoply can you share the @Enalmada version with your edits? I need to use it in production as well.

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Sep 9, 2018

The version we have is deeply integrated into our application. What I mean by this is certain actions within the application, reset and/or set preload cache, I need to find the time to break it apart is make it more universal for others to use.

@daau

This comment has been minimized.

Copy link

@daau daau commented Nov 22, 2018

For the curious, here's an example of a Rails app that uses a modified version of instantclick instead of turbolinks. I couldn't believe that it was a server side rendered Rails app when I first used it.

The source code is available here.

@danielcompton

This comment has been minimized.

Copy link

@danielcompton danielcompton commented Nov 22, 2018

I couldn't believe that it was a server side rendered Rails app when I first used it.

Heh, when I first saw dev.to I thought "why did they bother making this a SPA? It's just a static content site, server-side rendering would have been just fine".

@hopsoft

This comment has been minimized.

Copy link

@hopsoft hopsoft commented Nov 27, 2018

I've updated the code I've been using for this. Note: This solution works with Safari also. https://gist.github.com/hopsoft/ab500a3b584e2878c83137cb539abb32

@domchristie

This comment has been minimized.

Copy link
Collaborator

@domchristie domchristie commented Jan 2, 2019

Just throwing GoogleChromeLabs/quicklink into the mix. It uses a different approach in that it prefetches all links in the viewport, but has some nice features which might be worth integrating:

  • Waits until the browser is idle (using requestIdleCallback)
  • Checks if the user isn't on a slow connection (using navigator.connection.effectiveType) or has data-saver enabled (using navigator.connection.saveData)
@grosser

This comment has been minimized.

Copy link

@grosser grosser commented Jan 2, 2019

@domchristie

This comment has been minimized.

Copy link
Collaborator

@domchristie domchristie commented Jan 2, 2019

Prefetching all links sounds kinda wasteful especially on larger pages

I agree, and the hoverintent solutions above are certainly preferable to quicklink on its own. However, the solutions might be enhanced with the quicklink features listed above. I'm thinking about touch devices in particular. hoverintent relies on mouse position, so it has no benefit to touch devices. However, on a small touch screen, where fewer links are visible, quicklink might come in handy.

@jpic

This comment has been minimized.

Copy link

@jpic jpic commented Jan 2, 2019

@hopsoft does it still do two requests instead of one ?
I wonder if ServiceWorker is not the best way to prevent the second request. I've done some service worker experiments with a non-turbolinks project and it works extremely well.

@sfusato

This comment has been minimized.

Copy link

@sfusato sfusato commented Feb 10, 2019

The developer of InstantClick also released a newer, lighter plugin called InstantPage:
https://github.com/instantpage/instant.page

What would be the best way to integrate this with Turbolinks?

EDIT: I've just noticed @david-a-wheeler referenced this above. Sorry for duplicating this.

@hopsoft

This comment has been minimized.

Copy link

@hopsoft hopsoft commented Apr 13, 2019

@jpic I've since updated the prefetch script I'm using. https://gist.github.com/hopsoft/ab500a3b584e2878c83137cb539abb32

The prefetcher only makes 1 request. It ensures that clicking a prefetched url is handled as a Turbolinks restoration visit. A 2nd request is still made to pick up any page changes since the prefetch cached the Turbolinks snapshot... as per normal Turbolinks behavior.

Perceived load time is very fast as it is with any Turbolinks restoration visit.

I should reiterate that this solution has no dependencies and also works in Safari.

@david-a-wheeler

This comment has been minimized.

Copy link

@david-a-wheeler david-a-wheeler commented Apr 13, 2019

@hopsoft .. cool! Can you turn that into a pull request to be included in turbolinks? As long as it can be controlled on a per link basis, I think that would be worth doing.

@charnould

This comment has been minimized.

Copy link

@charnould charnould commented Aug 7, 2019

Any update on this pull request?
It would be super nice to have this feature in Turbolinks.
Thanks

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Aug 9, 2019

@hopsoft this is clean. Very clean.

@frederikhors

This comment has been minimized.

Copy link

@frederikhors frederikhors commented Sep 8, 2019

@hopsoft your work (here: #313 (comment)) is amazing.

@sstephenson, @packagethief, @domchristie can we PR that code in a useful feature requested by community for years now?

Not everything must always be perfect immediately, I believe, but if we start today then fix/nth PRs will come tomorrow.

Thanks for your work.

@dieulot

This comment has been minimized.

Copy link

@dieulot dieulot commented Sep 9, 2019

Author of instant.page here. Wouldn’t using both Turbolinks and instant.page satisfy your need? I don’t see why they wouldn’t be compatible.

There’s also an advantage to using them separately: instant.page is loaded after everything else, so it doesn’t slow down the initial page load.

@senordelaflor

This comment has been minimized.

Copy link

@senordelaflor senordelaflor commented Sep 10, 2019

@jpic I've since updated the prefetch script I'm using. https://gist.github.com/hopsoft/ab500a3b584e2878c83137cb539abb32

The prefetcher only makes 1 request. It ensures that clicking a prefetched url is handled as a Turbolinks restoration visit. A 2nd request is still made to pick up any page changes since the prefetch cached the Turbolinks snapshot... as per normal Turbolinks behavior.

Perceived load time is very fast as it is with any Turbolinks restoration visit.

I should reiterate that this solution has no dependencies and also works in Safari.

@jpic are you using this in production? love it!

@hopsoft

This comment has been minimized.

Copy link

@hopsoft hopsoft commented Sep 10, 2019

I've got it in production on a few apps. So far so good.

@frederikhors

This comment has been minimized.

Copy link

@frederikhors frederikhors commented Sep 26, 2019

@hopsoft, let can be changed to const on lines 6, 22, 48.

'url' is never reassigned. Use 'const' instead.eslint(prefer-const).

@panoply

This comment has been minimized.

Copy link

@panoply panoply commented Sep 26, 2019

While this feature is great and I've used a variation of it in a past project bundling it with Turbolinks might not be the best idea and I can understand why maintainers would be hesitant.

Sometimes features like this become troublesome when being used irresponsibly. Applying it to elements that users are hovering but not clicking like a products page where scrolling, stopping and hovering is abundant leads to countless requests being processed in vain and though this can be prevented with conditional execution using data attributes and timeouts, developers (especially those new to the game) tend to care little for the cost of speed increases.

One thing about projects like Turbolinks and Stimulus I've noticed is how vigorously thought through in both design and functionality they are. It would be interesting to see if something like this is merged or extended upon.

@david-a-wheeler

This comment has been minimized.

Copy link

@david-a-wheeler david-a-wheeler commented Sep 26, 2019

@dieulot - I've been assuming that Turbolinks and instant.click are not compatible, since Turbolinks fundamentally changes click behavior and maintains its own cache. If they can simply be installed simultaneously that'd be great, but I don't want to assume that.

@mayrsascha

This comment has been minimized.

Copy link

@mayrsascha mayrsascha commented Jan 1, 2020

FYI there is a thread on instant.page 's issue tracker regarding compatibility with Turbolinks instantpage/instant.page#52

@feliperaul

This comment has been minimized.

Copy link

@feliperaul feliperaul commented Mar 18, 2020

@hopsoft Thank you so much for your snippet. But one question: aren't you getting the security warnings on Rails 5.2 ?

I'm getting:

Security warning: an embedded <script> tag on another site requested protected JavaScript. If you know what you're doing, go ahead and disable forgery protection on this action to permit cross-origin JavaScript embedding

I don't understand why it's triggering this tough, since it's same origin.


UPDATE: Found out what the problem was. It was prefetching a controller action that had a response block like this:

respond_to do |format|
  format.js
  format.html
end

And the controller was, as expected, serving the .js.erb view instead of the .html.erb one, since the request was being made by the prefetch script using XHR.

To fix this, I just added a xhr.setRequestHeader('Accept', 'text/html'); inside the fetchPage function. I'm posting that on the gist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.