Major: Rework exports; Add `throttle` and `limit` options #120

lukeed · 2019-08-22T08:05:07Z

Sorry for the big diff; I was zoning & not really paying attention to git.

TLDR:

adds limit option – total allowed requests (Option to limit the maximum number of prefetched requests #4)
adds throttle option – max concurrency control via throttles (Option to limit the maximum number of prefetched requests #4 (comment))
removes urls option (temp?)
exports prefetch method directly
exports previous default as listen method
aborts listen early if no IO support
only require IO support when using listen
when okay, listen returns a reset / unlisten function

Code Size

// Before
        804 B: quicklink.js.gz
        667 B: quicklink.js.br
        804 B: quicklink.mjs.gz
        668 B: quicklink.mjs.br
        877 B: quicklink.umd.js.gz
        729 B: quicklink.umd.js.br

// After
        871 B: quicklink.js.gz
        734 B: quicklink.js.br
        878 B: quicklink.mjs.gz
        741 B: quicklink.mjs.br
        949 B: quicklink.umd.js.gz
        798 B: quicklink.umd.js.br

Notes:

I realized we were using the toPrefetch set somewhat incorrectly. We did a bunch of work to figure out if we should "queue" the link's href value, and then, once the link was intersecting, we did more work to see if the link.href was approved, and then finally did more work to clear the queue.

That's a lot of work.

Instead, it was much simpler to do the first set of work to see if we should observe the link in the first place! Then anything else the IO is processing is already okay'd for processing.

This leaves the toPrefetch set as a track record of what has been called, which is identical to what this makeshift cache was doing, too.

Thirdly, these individually-operating caches were individually operating, which means that when you were brave enough to venture into quicklink/src/prefetch.mjs, you were prefetching things that (a) may have already been prefetched; and/or (b) prefetching things that your IO won't ever know about.

The best way to solve this was to get everything in the same file and operating on the same cache list. It also allows for easy access to the prefetch function w/o manually traversing the module contents #98 (comment) which actually prevented code reuse too (quicklink was bundled, but reached back in for another copy of source code).

Finally, I removed the urls option because of the easily accessible prefetch export. It feels redundant and very out of place now that the "Go get this" and the "Please listen to that" behaviors have been separated. Of course, I can revert this if it should be kept.

TODOs

Add tests for new features (how to test throttle?)
Add/Update documentation

Closes #4
Closes #79
Closes #118
Unblocks #98

- add `throttle` option - add `limit` option - export `prefetch` directly - export old `default` as `listen` method - remove `urls` option (temp?) - return early if no IO support - return `reset` function if okay

src/index.mjs

lukeed · 2019-08-22T08:12:25Z

src/index.mjs

+    timeout: options.timeout || 2e3
+  });
+
+  return function () {


This is good for SPA environments, where listen is initially called on a parent container and then any page/route-change can dump the contents of what was relevant to the previous page.

addyosmani · 2019-08-23T15:13:38Z

Finally, I removed the urls option because of the easily accessible prefetch export. It feels redundant and very out of place now that the "Go get this" and the "Please listen to that" behaviors have been separated. Of course, I can revert this if it should be kept.

I agree with the rationale for removal, but do have some questions. If we drop the urls option, users would need to iterate through a supplied array of URLs they want to prefetch manually, right? i.e similar to the current prefetch import behavior if using it as a standalone:

const urls = ['1.html', '2.html'];
const promises = urls.map(url => Quicklink.prefetch(url));
Promise.all(promises);

I would be supportive of dropping urls if we modified the prefetcher to accept either a single URL string or an array of URLs (to preserve ergonomics and ease upgrading for anyone switching to this version). Wdyt?

addyosmani · 2019-08-23T15:22:47Z

Thank you for working on these changes, @lukeed!

The best way to solve this was to get everything in the same file and operating on the same cache list. It also allows for easy access to the prefetch function w/o manually traversing the module contents #98 (comment) which actually prevented code reuse too (quicklink was bundled, but reached back in for another copy of source code).

A shift to working against the same cache list was a good call. I'm also glad to hear you discovered a set of changes to improve code reuse when we've finally bundled Quicklink.

Sorry for the big diff; I was zoning & not really paying attention to git.

No worries. I'm happy to treat this PR as the next major release candidate once we are further along in the reviews. Given the breaking changes in the API surface, we should probably consider this Quicklink 2.0.0. Top of mind is making sure there's feature parity in this new version. Looking forward to the tests :)

addyosmani · 2019-08-23T15:31:46Z

src/index.mjs

 */
-export default function (options) {
+export function listen(options) {


Could you talk a little about the thought process of switching from a default to using listen? In practice, I think most users are likely to copy/paste the default code snippet we provide (and so I don't view this as a huge ergonomics change), but would be good to walk through reasoning :)

Maybe there's not as much consensus as I think there is, but I think have mixed imports vs default-only and/or named-only is the trickiest of the three. So, for me it basically comes down to convention/simplicity. It's certainly not impossible to have mixed exports.

However, for UMD and CJS users, it does mean you have to call "listen" function via quicklink.default()

src/index.mjs

addyosmani · 2019-08-23T15:37:39Z

src/index.mjs

+* @param {Object} [conn] - navigator.connection (internal)
+* @return {Object} a Promise
+*/
+export function prefetch(url, isPriority, conn) {


👍 to exposing prefetch like this.

I am excited about is us enabling Quicklink to be viewed as both a library with a heuristic for prefetching but also as just a generally good prefetcher. I believe hiding prefetch.mjs behind our abstraction likely led to users thinking you had to use our heuristic to take advantage of any of the prefetching behavior at all, which isn't the case.

addyosmani

Very enthusiastic about the changes here so far. Left some comments along the way :)

lukeed · 2019-08-23T18:17:29Z

I would be supportive of dropping urls if we modified the prefetcher to accept either a single URL string or an array of URLs (to preserve ergonomics and ease upgrading for anyone switching to this version). Wdyt?

I was envisioning the a Promise.all() from userland to achieve this. It doesn't really matter to me, but my thinking was that if they cared about the responses, they're going to have to loop the return array anyway

lukeed · 2019-08-23T18:27:06Z

Pushed up some changes, including a look at having prefetch accept multiple URLs.

Once we have the changelist ironed out, I think I'll wrap this up with a set of test & doc updates.

These are the updated build stats:

        914 B: quicklink.js.gz
        777 B: quicklink.js.br
        921 B: quicklink.mjs.gz
        789 B: quicklink.mjs.br
        992 B: quicklink.umd.js.gz
        842 B: quicklink.umd.js.br

addyosmani · 2019-08-23T20:13:13Z

Pushed up some changes, including a look at having prefetch accept multiple URLs.

Thank you, @lukeed!

addyosmani · 2019-08-24T20:12:52Z

Catching up on the latest discussions, I believe we now have quorum? :) Thanks again. If there's anything design-wise you're waiting on further input for please feel free to reach out anytime.

I think moving on to tests and docs (whenever you have time) should feel unblocked.

lukeed · 2019-09-04T09:08:48Z

Hey hey, here are final build stats

Build "quicklink" to dist:
        906 B: quicklink.js.gz
        763 B: quicklink.js.br
        910 B: quicklink.mjs.gz
        775 B: quicklink.mjs.br
        987 B: quicklink.umd.js.gz
        837 B: quicklink.umd.js.br

Not sure how to test throttle – not familiar enough with all puppeteer APIs. Follow up with docs in the morning~

addyosmani · 2019-09-05T19:27:02Z

Thanks for working through these changes, @lukeed! Tests LGTM so far.

lukeed · 2019-09-08T21:47:44Z

Hey, sorry for the delay on this one 🙈

Good news is that I figured how to test the throttling~!
Updated the readme too.

Let me know if you need anything to be changed. Now that the bulk of the work is out of the way, anything else should be much quicker, even with my current forever-catchup schedule.

lukeed · 2019-09-08T22:31:27Z

Hey @addyosmani, I reformatted the README in a way that reads easier to my eyes – I felt like the listen() docs were crammed, especially with the addition of the prefetch() text behind it.

They are here (lukeed#1 – rendered). If you like them, I'll merge that PR which will update this guy.

addyosmani · 2019-09-12T02:22:49Z

Rendered README LGTM. Thanks for reworking it, Luke! Just read through.

lukeed · 2019-09-12T16:48:10Z

Cool! Merged the README changes

addyosmani · 2019-09-12T18:52:07Z

Thank you, Luke! As far as I'm concerned, reviewing through the full set of changes I think this is in a good place to land it in master 🚢

I'm thinking that we do this, tag the release as an alpha and invite developers to try out the new API. We can then rework the upcoming site for the project to account for API changes and launch the new major version + site + updated demos at the same time.

Sound reasonable?

lukeed · 2019-09-12T19:01:31Z

Yup, sounds good! Thanks for all the continuous feedback

addyosmani

LGTM

addyosmani · 2019-09-13T20:26:04Z

W00. It's landed. Thanks, Luke!

lukeed added 6 commits August 22, 2019 00:25

[major] rewrite w/ new exports & options;

66ccc7a

- add `throttle` option - add `limit` option - export `prefetch` directly - export old `default` as `listen` method - remove `urls` option (temp?) - return early if no IO support - return `reset` function if okay

chore: remove static-url-list test

5647884

chore: update usage

b7eb56c

golf: shave 4B ~

65cf4d3

fix: xbrowser throw on set.size getter

f53b143

chore: shuffle "main" location

435393c

lukeed commented Aug 22, 2019

View reviewed changes

src/index.mjs Outdated Show resolved Hide resolved

lukeed commented Aug 22, 2019

View reviewed changes

src/index.mjs Outdated Show resolved Hide resolved

lukeed commented Aug 22, 2019

View reviewed changes

This was referenced Aug 23, 2019

ignore configuration filter can be bypassed by links sharing survivors urls #78

Closed

Consider adding rootMargin to the Intersection Observer #14

Open

addyosmani reviewed Aug 23, 2019

View reviewed changes

src/index.mjs Outdated Show resolved Hide resolved

addyosmani reviewed Aug 23, 2019

View reviewed changes

addyosmani requested changes Aug 23, 2019

View reviewed changes

lukeed added 2 commits August 23, 2019 11:13

feat: add onError option for prefetch catch

46e5faa

fix: expand timeout

5b98337

feat: allow prefetch to accept url[] list

ee65953

lukeed added 3 commits September 3, 2019 22:09

fix: remove TODO notes

fb65441

golf: always handle prefetch as array

e25e217

chore: complete tests

27d3a34

chore: add throttle tests

f410264

chore: update docs

2cce45b

fix: separate listen() vs prefetch() callers

6d1e112

docs: format API docs differently (#1)

405b1c0

addyosmani approved these changes Sep 13, 2019

View reviewed changes

addyosmani merged commit 4044de0 into GoogleChromeLabs:master Sep 13, 2019

lukeed deleted the feat/throttle branch September 13, 2019 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major: Rework exports; Add `throttle` and `limit` options #120

Major: Rework exports; Add `throttle` and `limit` options #120

lukeed commented Aug 22, 2019 •

edited

Loading

lukeed Aug 22, 2019

addyosmani Aug 23, 2019

addyosmani commented Aug 23, 2019 •

edited

Loading

addyosmani commented Aug 23, 2019

addyosmani Aug 23, 2019

lukeed Aug 23, 2019

addyosmani Aug 23, 2019

addyosmani left a comment

lukeed commented Aug 23, 2019 •

edited

Loading

lukeed commented Aug 23, 2019 •

edited

Loading

addyosmani commented Aug 23, 2019

addyosmani commented Aug 24, 2019

lukeed commented Sep 4, 2019

addyosmani commented Sep 5, 2019

lukeed commented Sep 8, 2019

lukeed commented Sep 8, 2019

addyosmani commented Sep 12, 2019

lukeed commented Sep 12, 2019

addyosmani commented Sep 12, 2019

lukeed commented Sep 12, 2019

addyosmani left a comment

addyosmani commented Sep 13, 2019

Major: Rework exports; Add throttle and limit options #120

Major: Rework exports; Add throttle and limit options #120

Conversation

lukeed commented Aug 22, 2019 • edited Loading

lukeed Aug 22, 2019

Choose a reason for hiding this comment

addyosmani Aug 23, 2019

Choose a reason for hiding this comment

addyosmani commented Aug 23, 2019 • edited Loading

addyosmani commented Aug 23, 2019

addyosmani Aug 23, 2019

Choose a reason for hiding this comment

lukeed Aug 23, 2019

Choose a reason for hiding this comment

addyosmani Aug 23, 2019

Choose a reason for hiding this comment

addyosmani left a comment

Choose a reason for hiding this comment

lukeed commented Aug 23, 2019 • edited Loading

lukeed commented Aug 23, 2019 • edited Loading

addyosmani commented Aug 23, 2019

addyosmani commented Aug 24, 2019

lukeed commented Sep 4, 2019

addyosmani commented Sep 5, 2019

lukeed commented Sep 8, 2019

lukeed commented Sep 8, 2019

addyosmani commented Sep 12, 2019

lukeed commented Sep 12, 2019

addyosmani commented Sep 12, 2019

lukeed commented Sep 12, 2019

addyosmani left a comment

Choose a reason for hiding this comment

addyosmani commented Sep 13, 2019

Major: Rework exports; Add `throttle` and `limit` options #120

Major: Rework exports; Add `throttle` and `limit` options #120

lukeed commented Aug 22, 2019 •

edited

Loading

addyosmani commented Aug 23, 2019 •

edited

Loading

lukeed commented Aug 23, 2019 •

edited

Loading

lukeed commented Aug 23, 2019 •

edited

Loading