GoogleChromeLabs / quicklink

⚡️Faster subsequent page-loads by prefetching in-viewport links during idle time
https://getquick.link
Apache License 2.0
11.02k stars 403 forks source link

Prefetch links from same domain? #31

Closed maxigimenez closed 5 years ago

maxigimenez commented 5 years ago

Maybe set a guard to prefetch urls with the same domain? Because now either XHR or Fetch will fail for external links

naeluh commented 5 years ago

This worked for me but yes I agree

 let links = [...document.getElementsByTagName('a')].filter(a => {
    return a.hostname === window.location.hostname;
  });
  quicklink({urls:links});
addyosmani commented 5 years ago

Reviewing some of the feedback from Twitter, quite a few folks have asked if there's a way to guard prefetching to same origin. That's a pretty fair request.

I can see a few ways this could be done..

  1. Same-origin is the default for all forms of prefetching (rel=prefetch, fetch(), XHR etc)
  2. Same-origin guarding is an option (e.g quicklinks({ sameDomainOnly: true }))
  3. Same-origin guarding is a documented pattern in the README/FAQ (similar to @naeluh's POC)

@lukeed would also love your take on this if you have thoughts on defaults :)

lukeed commented 5 years ago

I think whitelist or allowed option would cover most use cases. It'd be an array of all domain strings the quicklink is permitted to fetch. We can ask users to provide their own domain in this list, or add an additional option (sameDomainOnly) that pushes into that list.

let allowed = [];
const observers = new IO(...)

export default function (opts) {
  opts = Object.assign({
    // ...
    whitelist: [],
  }, opts);

  let allowed = opts.sameDomainOnly ? [location.hostname] : opts.whitelist;

  // .... we should trust the options.urls given

  } else {
      Array.from(options.el.querySelectorAll('a'), link => {
        observer.observe(link);
        // <a> tags already parse URL bodies for us~!
        // empty "allowed" ~> everything OK
        let isAllowed = !allowed.length || allowed.includes(link.hostname);
        isAllowed && toPrefetch.add(link.href);
      });
  }
}
Avi98 commented 5 years ago

I would like to work on this if it's not taken

lukeed commented 5 years ago

Oops, sorry @Avi98! 🙈 Just pulled the above into a PR

addyosmani commented 5 years ago

Thanks, @lukeed! I'm in favor of a whitelist option with sameDomainOnly piggy-backing off that implementation. I see you've already filed a PR with your POC above :)

I was wondering if it was premature to model whitelisted URLs around regular expression patterns (similar to Workbox. Though, perhaps opting for link.hostname checking initially is sufficient until we've had requests for more complex URL pattern matching.

lukeed commented 5 years ago

You bet! I hear ya – hesitated and thought to add the full-fledged capabilities out of the gate, but it may not even be requested. We did a similar thing in Sapper & this was the smallest form I was able to find.

addyosmani commented 5 years ago

Whoa. TIL you contributed to Sapper too. Nice :)

lukeed commented 5 years ago

There's something infectious about @Rich-Harris