ampproject / amp-sw

A drop in service worker library to help your AMP pages gain network resiliency in 1 line
Apache License 2.0
70 stars 29 forks source link

Evaluating link-prefetch module vs Quicklink #15

Open westonruter opened 5 years ago

westonruter commented 5 years ago

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: https://github.com/ampproject/amphtml/issues/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 commented 5 years ago

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

AMP Component

prateekbh commented 5 years ago

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

================ 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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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.