curriculum icon indicating copy to clipboard operation
curriculum copied to clipboard

Feature: SW revision

Open SallyMcGrath opened this issue 2 years ago • 8 comments

Not the run forward cache (feel free to build this) but

  • some revisions from Ali's code review on #134 eg rm pointless script call in head
  • move to stale while revalidate cache strategy
  • rm unused image cache limit
  • skip caching external images to reduce CORS problems
  • fix cloney clone error
  • everything else largely the same

SallyMcGrath avatar Jun 26 '23 14:06 SallyMcGrath

Deploy Preview for cyf-curriculum ready!

Name Link
Latest commit e1287474793b5be0ac8c8fe513a49b41272f949c
Latest deploy log https://app.netlify.com/sites/cyf-curriculum/deploys/64999a3824f2c20008243810
Deploy Preview https://deploy-preview-153--cyf-curriculum.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Jun 26 '23 14:06 netlify[bot]

Sorry just getting round to this. I don't really understand what this PR is doing - do you want someone to review with more context? I don't even know how to check it is supposed to be doing the thing.

Dedekind561 avatar Jun 29 '23 07:06 Dedekind561

Sorry just getting round to this. I don't really understand what this PR is doing - do you want someone to review with more context? I don't even know how to check it is supposed to be doing the thing.

test by looking in Application > Service Worker

sw.js : activated and is running

check Application > Storage > Cache Storage > CYF caches exist

browse around and then turn off Network access, either by killing your connection or checking Service Workers > Offline

Open Console and check the logs - the sw should not generate errors as you browse around. Errors noted from previous PR:

 TypeError: Failed to execute 'clone' on 'Response': Response body is already used

I was consuming the response body twice in some way previously.

The FetchEvent for "http://i.ytimg.com/vi/pBy1zgt0XPc/maxresdefault.jpg" resulted in a network error response: an object that was not a Response was passed to respondWith().
Promise.then (async)

I was triggering a CORS error by attempting to cache external images (from youtube), but really I only want to cache same origin images.

So this part of the code tests the origin and only caches our own images:

// Bypass cache for external image requests
  if (requestUrl.origin !== location.origin && /\.(jpe?g|png|webp)$/.test(requestUrl.pathname)) {
    event.respondWith(fetch(event.request));
    return;
  }

Those were my main errors. I had also set an image cache limit constant and then never called it.

SallyMcGrath avatar Jun 30 '23 07:06 SallyMcGrath

I also did wonder if using something like Workbox would be beneficial?

40thieves avatar Jul 01 '23 13:07 40thieves

I don't think it's worth it - like - I don't want to build in a massive build step at all. (I'd rather have no SW than a larger build step.)

It's only a small thing to make connection loss a bit less painful.

I am considering dropping Google fonts and serving a subsetted font instead. What do you think?

In general, instead of finding ways to complicate and elaborate the build, let's try to make it simpler and more robust.

SallyMcGrath avatar Jul 01 '23 13:07 SallyMcGrath

Yis me too. Two years since I wrote a SW!!

On Sat, 1 Jul 2023 at 15:13, Alasdair Smith @.***> wrote:

@.**** commented on this pull request.

Seems like it works (better than previously), but not quite how I'd expect

In static/sw.js https://github.com/CodeYourFuture/curriculum/pull/153#discussion_r1248822159 :

  •      if (/\.woff2?$|\.ttf$/.test(requestUrl.pathname)) {
    
  •        cacheName = FONT_CACHE_CYF;
    

It appears that font caching isn't working both in the Netlify preview and locally for me.

For some reason in the Netlify preview, it appears the font cache is never initiated: [image: Screenshot 2023-07-01 at 11 37 07] https://user-images.githubusercontent.com/424411/250268955-6c408855-bed0-46ae-af12-1afaddc5cad2.png

Locally, I do see a font cache, but it's empty: [image: Screenshot 2023-07-01 at 11 39 03] https://user-images.githubusercontent.com/424411/250269106-1d6f31fb-0211-4662-991b-398acfb70b6b.png

It's not entirely clear to me why this is, but I think it's related to us loading via Google Fonts - since GF loads the actual font files as a second step (i.e. @.@.;600&display=swap will then request https://fonts.gstatic.com/s/spacegrotesk/v15/V8mDoQDjQSkFtoMM3T6r8E7mPbF4C_k3HqU.woff2 )

In static/sw.js https://github.com/CodeYourFuture/curriculum/pull/153#discussion_r1248828215 :

     });
  •  })
    
  •  return fetchPromise || cachedResponse;
    

It's been a long time since I studied the SW API, but wouldn't this give preference to the fetchPromise over the cache response?

If i understand correctly, that means we'd only serve the cache if the network fails - which seems quite similar (the same?) to what we were doing before?

— Reply to this email directly, view it on GitHub https://github.com/CodeYourFuture/curriculum/pull/153#pullrequestreview-1508616481, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKB7SDQCK3RFNGPNAW4SBGLXOAPBPANCNFSM6AAAAAAZUGZHKE . You are receiving this because you authored the thread.Message ID: @.***>

-- Best,

Sally

SallyMcGrath avatar Jul 01 '23 13:07 SallyMcGrath

I don't think it's worth it

That's fair 👍🏻 I imagine it would be a pain to integrate a Node dep into the Hugo system.

My thought process was mostly: SWs are not super easy to write and so relying on an abstraction layer written by experts seems sensible. But totally can see the other side of the argument.

I am considering dropping Google fonts and serving a subsetted font instead.

I'd be onboard with that.

There's a perf argument too: there's not too much value in relying on "public" CDNs like Google Fonts these days since browser caches are isolated.

40thieves avatar Jul 01 '23 13:07 40thieves

test by looking in Application > Service Worker

  • [x] sw.js : activated and is running

  • [x] check Application > Storage > Cache Storage > CYF caches exist

  • [x] browse around and then turn off Network access, either by killing your connection or checking Service Workers > Offline

  • [x] Open Console and check the logs - the sw should not generate errors as you browse around. Errors noted from previous PR:

I'm only seeing the error when network is turned off and SW goes to the cache, so this seems all good to me.

I'm getting similar issues with fonts as discussed above.

But all seems to be working on my local machine.

Dedekind561 avatar Jul 02 '23 14:07 Dedekind561

stale - start again if you wish to pursue it

SallyMcGrath avatar Jul 13 '24 09:07 SallyMcGrath