Allow using preloading over prefetching. #2388
Conversation
| @@ -135,6 +145,20 @@ class Preconnect { | |||
| return false; | |||
| } | |||
|
|
|||
| /** @private */ | |||
| isPreloadSupported_() { | |||
There was a problem hiding this comment.
Memoizing would really help here, avoiding a bunch of useless <link>s from being created.
There was a problem hiding this comment.
So this is still only memoized per instance of Preconnect. Can we move it up into a script variable instead?
There was a problem hiding this comment.
There is only one instance (per window).
|
Looks great! |
|
I've addressed the comments. I am going to work on refactoring the usage to pass the |
| @@ -135,6 +144,20 @@ class Preconnect { | |||
| return false; | |||
| } | |||
|
|
|||
| /** @private */ | |||
| isPreloadSupported_() { | |||
| if (this.preloadSupported_ !== undefined) { | |||
There was a problem hiding this comment.
If you cache the result in an instance var, then don't do this, just call it once and use the instance var after.
|
PTAL. Added tests and updated usage to pass the |
mkhatib
changed the title
| expect(iframe.doc.querySelector('link[rel=preconnect]').href) | ||
| .to.equal('https://a.prefetch.com/'); | ||
| // Actual prefetch | ||
| const fetches = iframe.doc.querySelectorAll( |
There was a problem hiding this comment.
This is a bit unfortunate in that this test will fail when travis upgrades to a Chrome that supports preload.
I think it is better to remove this test and instead think about writing a test for isPreloadSupported_ that mocks stubs document.createElement('link')
There was a problem hiding this comment.
I had that in mind and I was thinking of always mocking the perloadSupported=false except when I am testing for preloading. What do you think? So basically this will change the preloadSupported=undefined to false in the beforeEach block?
There was a problem hiding this comment.
Sounds good. One think I typically do is have 1 test that does not mock the feature test, but succeeds independent of whether the tested for thing is present or not.
There was a problem hiding this comment.
Done! The way I did this is basically for the existence of either preload or prefetch links after disabling the mock preloadSupported=undefined.
|
Looks great. Some test feedback |
|
LGTM |
Allow using preloading over prefetching.
Would something like this work? Or should we have two separate methods for prefetch and preload? The similarities between both kinda begs for one method.
Addresses #2372