-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature: Service worker (custom Astro integration) #242
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # src/components/PerfHead/PerfHead.astro
Deploying head-start with Cloudflare Pages
|
@@ -0,0 +1,64 @@ | |||
import type { APIRoute, AstroIntegration } from 'astro'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would move this file to config/astro/service-worker-integration.ts
or config/astro/integrations/service-worker.ts
, since it's basically a configuration snippet that's loaded into the main astro.config.ts
. Whereas /scripts/
contains all scripts that are executed from a cli context.
import { fileURLToPath } from 'node:url'; | ||
import * as esbuild from 'esbuild'; | ||
|
||
const serviceWorkerSrc = fileURLToPath(new URL('../src/assets/service-worker.ts', import.meta.url)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think within an Astro integration you have access to the configured src and dist directory (srcDir
and outDir
on the config
param) (reference).
So I would change the API of the serviceWorkerIntegration
so it can be called as something like:
serviceWorkerIntegration({
srcFilename: 'assets/service-worker.ts', // relative to `srcDir`,
outFilename: 'service-worker.ts', // relative to `outDir`,
})
import { CacheableResponsePlugin } from 'workbox-cacheable-response'; | ||
import { ExpirationPlugin } from 'workbox-expiration'; | ||
|
||
((self: ServiceWorkerGlobalScope) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can be simplified by declaring self
?:
/// <reference lib="webworker" />
declare const self: ServiceWorkerGlobalScope;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at this with @decrek and there are two ways to do it:
declare const self: ServiceWorkerGlobalScope;
export default null // or export default {}
or do the self-invoking function to assign it to a new similarly named variable.
Without the export statement, it'll throw errors. Since the service-worker is specifically not meant to be imported, the self-invoking function made more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I read that in fact. I would opt for declare
+ export default null
(with a comment). Since I think the expected behaviour is that you can write self.add...
in the root. So I think we should write the plumbing to make that work.
@@ -20,3 +20,17 @@ const fontFaceDeclaration = fonts.map((font) => getFontFaceDeclaration(font)).jo | |||
)) | |||
} | |||
<style set:html={fontFaceDeclaration}></style> | |||
|
|||
<script is:inline> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PerfHead sounds like the perfect location for this :)
Changes
Associated issue
Resolves #199
How to test
Checklist