-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add web-vitals integration #10883
Add web-vitals integration #10883
Conversation
Inferred type was causing an error which couldn’t be ignored: The inferred type of 'AstrojsWebVitals_Metric' cannot be named without a reference to '../node_modules/@astrojs/db/dist/_internal/core/types.js'. This is likely not portable. A type annotation is necessary.
This reverts commit e93cfe6.
🦋 Changeset detectedLatest commit: 23bbe95 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Ready for review! Testing is a little bit minimal. Not sure if we’d want an end-to-end test for the client script.
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.
This middleware preserves streaming and injects a <meta>
tag to each page’s <head>
. Wouldn’t mind someone taking a quick look to make sure there are no footguns here.
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.
Looks great! The logic seems good to me.
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.
Is this file intended to be committed?
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 — currently Astro DB types aren’t working great for integrations that don’t have an Astro project environment to refer to, so we’re kind of working around that with this triple-slash reference.
Hopefully something @bholmesdev or a TS magician might be able to improve in the future?
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.
Implementation looks robust and well-tested. A vital
addition to Astro
})); | ||
const body = JSON.stringify(rawBody); | ||
const endpoint = '/_/astro-vitals'; | ||
if (navigator.sendBeacon) navigator.sendBeacon(endpoint, body); |
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.
TIL about sendBeacon
💡
return str.length > 1 && str.at(-1) === '/' ? str.slice(0, -1) : str; | ||
} | ||
|
||
function miniEncodeAttribute(str: string) { |
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.
We sure this covers all necessary cases?
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’m pretty sure for attributes it does? I think in fact even <
and >
would be safe unencoded — just tried in an Astro component and this input:
content={'/blog/&"<>\\\'/'}
output to
content="/blog/&"<>\'/"
So only the &
and the "
were escaped.
Changes
@astrojs/web-vitals
integrationThis branch currently includes some changes to@astrojs/db
from ImproveasDrizzleTable()
types #10882 that will go away once that is mergedTesting
Added some basic tests for dev mode. Not quite sure what a build test would look like or if we should have end-to-end tests for the client-side script?
Docs
Minimal docs in the README for now cc @withastro/maintainers-docs