Normalize URLs before checking the toPrefetch set

[mathiasbynens]

For example, these three URLs are all equivalent:

https://www.google.com
https://www.google.com/
https:\\www.google.com/

(That last one is an extreme example, but it’s pretty common to see the first and second variation used interchangeably.)

When reading an element’s .href like in

quicklink/src/index.mjs

Line 25 in f9a2ca7

const url = entry.target.href;

, you’d get the following normalized URL for each of those three:

https://www.google.com/

However, the same normalization is not applied when the user manually passes in URLs:

quicklink/src/index.mjs

Line 67 in f9a2ca7

options.urls.forEach(prefetcher);

Because of this, the toPrefetch set currently ends up with three separate entries in this case, even though all of them normalize to the same URL:

quicklink({
   urls: [
    'https://www.google.com',
    'https://www.google.com/',
    'https:\\www.google.com/',
  ],
});

It can be solved by forcing URL normalization before calling prefetcher on this line:

quicklink/src/index.mjs

Line 67 in f9a2ca7

options.urls.forEach(prefetcher);

-      options.urls.forEach(prefetcher);
+      options.urls.forEach(url => prefetcher(new URL(url).toString()));

[addyosmani]

This is a great catch. Nice work highlighting the normalization issue, @mathiasbynens ❤️ Given URL() has pretty decent cross-browser support, I'm happy for us to use your suggested change in some form. We probably want to sanity check if normalization should happen elsewhere in index.mjs.

Do you want to PR a fix? If not, happy to patch directly :)

[lukeed]

Nothing gets added to toPrefetch when you pass in an options.url list. The prefetcher only looks to remove Set entries – additions are made thru the "scraped" links which are already normalized as you mentioned.

That said, we should still probably normalize so that the prefetch itself can send valid requests. Eg, fetch('https:\\www.google.com/') will fail.

IMO, normalization should happen here: https://github.com/GoogleChromeLabs/quicklink/blob/master/src/index.mjs#L37

It'll be redundant for the "scraped" links, but it avoids adding a new function 😇

[addyosmani]

@lukeed Hah. Avoiding the addition of a new function is fair. Were you thinking of normalization around L37 looking similar to this?

function prefetcher(url) {
  const nURL = new URL(url).toString();
  toPrefetch.delete(nURL);
  prefetch(nURL, observer.priority);
}

[lukeed]

I blame Jason 🙃

Was thinking this:

function prefetcher(url) {
 toPrefetch.delete(url);
 prefetch(new URL(url).toString(), observer.priority);
}

Since anything that makes its way into toPrefetch is already normalized. If we introduce some way for option.urls to coexist within the Set, then yes, the whole Set has to be normalized on entry and/or removal

---

Edit: We can also just do prefetch(new URL(url), priority) since fetch() will accept an URL or string & I just discovered that XHR will also work with a URL instance. 🎉

[addyosmani]

Closed by #37. Thanks again!