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

Evaluating link-prefetch module vs Quicklink #15

Open
westonruter opened this issue Feb 10, 2019 · 9 comments
Open

Evaluating link-prefetch module vs Quicklink #15

westonruter opened this issue Feb 10, 2019 · 9 comments

Comments

@westonruter
Copy link
Member

In reviewing the link-prefetch module, I realized there is quite a bit of overlap with Quicklink. The link-prefetch module will prefetch links in the page that have the data-prefetch data-rel=prefetch attribute:

https://github.com/ampproject/amphtml/blob/0937333cb3f4d1b09bd41f86db565c2dcda7ed3a/extensions/amp-install-serviceworker/0.1/amp-install-serviceworker.js#L381-L407

However, it seems unfortunate to have to manually add an attribute to each link that you want to prefetch. It would seem much more user friendly to automatically prefetch links that are visible in the current viewport, which is what Quicklink enables. There's actually an open issue to add a Quicklink component to AMP: ampproject/amphtml#19905. The automatic prefetching of links in the current viewport is a sensible default which could be overridden with configuration.

I don't know the full history of link-prefetch module here, but to me it seems like it's a better fit for an AMP component rather than the service worker. Thoughts?

@kristoferbaxter
Copy link
Contributor

To start the conversation, let's list out the pros and cons for prefetching work in an AMP Component versus ServiceWorker. @prateekbh and @westonruter, please add in or correct me if I am misinterpreting.

ServiceWorker

  • PRO Off main thread: reduces interference with main thread JavaScript execution.
  • PRO Customizable caching behaviour via ServiceWorker cache.
  • PRO Customizable request headers.
  • CON Prefetching requires ServiceWorker integration.

AMP Component

  • PRO Simple to add to existing documents.
  • CON Additional configuration script in document.
  • CON Additional main thread code execution.

@prateekbh
Copy link
Member

prateekbh commented Feb 11, 2019

Just a few things to put em out there before any further discussion:

  • Currently for link prefetching we use a link prefetch when available to let the browser decide when its a good time to do the prefetching with lowest importance, we use service worker as a fallback for when link prefetch is not supported at all.

  • Another thing to note is that we currently do need some work on main thread to at-least look at the DOM, check which links are to be pre-fetched and tell the service worker to prefetch them.

================
Now, quicklink definitely adds a bunch of stuff before just prefetching those links and depends on a bunch of APIs(Intersection observer and requestIdleCallback) whose presence are not guaranteed and would need to be included in order for Quicklinks to work perfectly.

On the other hand a component of <amp-quicklink>(whether it uses quicklink or just amp's inViewportLifeCycle method) will give a very clear intent for its work and will save the user from any unnecessary fetching of links which they will never visit because the link never came in the viewport.

@prateekbh
Copy link
Member

Biggest PRO in my head for an amp-component would be:
If we implement <amp-quicklink> as an AMP component then the resource-manager can just take care of when to prefetch the link and save the user from prefetching any extra data.

@kristoferbaxter
Copy link
Contributor

we use service worker as a fallback for when link prefetch is not supported at all.

If we are only planning to use the ServiceWorker as a fallback, this gives additional coverage to Safari on iOS only right? Might be quite complicated to maintain for a single fallback.

Another thing to note is that we currently do need some work on main thread to at-least look at the DOM, check which links are to be pre-fetched and tell the service worker to prefetch them.

This limitation is disappointing, should we be writing a specification for inspecting document responses inside a Worker? My hunch is if we are planning to use the ServiceWorker to execute the work, it should be as much as possible off main thread (or the benefit is quite lessened).

@kristoferbaxter
Copy link
Contributor

Biggest PRO in my head for an amp-component would be:
If we implement <amp-quicklink> as an AMP component then the resource-manager can just take care of when to prefetch the link and save the user from prefetching any extra data.

I believe an implementation of <amp-quicklink> or similar shouldn't target API compatibility with the standard version... instead should implement the intended functionality with the minimal configuration required for the AMP restricted space.

@prateekbh
Copy link
Member

Totally agreed!
Something as simple as <amp-quicklink href="......" /> should do the needful.
We can definitely use some inspiration from Quicklink but does not need to stick to its API layer or implementation.

@kristoferbaxter
Copy link
Contributor

kristoferbaxter commented Feb 11, 2019

So, to be clear... we're aligned on create an AMP Component for this usecase and remove the functionality from the ServiceWorker code?

Pick a +1 or -1 to indicate!

@addyosmani
Copy link
Member

I believe an implementation of <amp-quicklink> or similar shouldn't target API compatibility with the standard version... instead should implement the intended functionality with the minimal configuration required for the AMP restricted space.

Fwiw, I'm highly supportive of this direction. The alignment on implementing similar functionality (even if not directly requiring standard compat for quicklink's API) is also par for the course of how we've seen a few other OSS projects approach integrations.

@demianrenzulli
Copy link

Hi folks,

This issue and the other related one haven't been updated for a while now. Can we expect that <amp-quicklink> component to come up at some point during Q1?

We were thinking that some companies working on AMP-First sites (e.g. publishers), would be interested on trying this.

I'm currently helping maintain the quicklink library, so If there's anything we can do to help, please, let us know.

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

No branches or pull requests

5 participants