Skip to content
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

Merged
merged 33 commits into from May 3, 2024
Merged

Add web-vitals integration #10883

merged 33 commits into from May 3, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented Apr 26, 2024

Changes

Testing

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

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.
Copy link

changeset-bot bot commented Apr 26, 2024

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Apr 26, 2024
Copy link
Member Author

@delucis delucis left a 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.

packages/integrations/web-vitals/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member Author

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.

@delucis delucis marked this pull request as ready for review April 29, 2024 17:25
@delucis delucis changed the title [WIP] Add web-vitals integration Add web-vitals integration Apr 29, 2024
Copy link
Member

@bluwy bluwy left a 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.

Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Contributor

@bholmesdev bholmesdev left a 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about sendBeacon 💡

packages/integrations/web-vitals/src/index.ts Outdated Show resolved Hide resolved
return str.length > 1 && str.at(-1) === '/' ? str.slice(0, -1) : str;
}

function miniEncodeAttribute(str: string) {
Copy link
Contributor

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?

Copy link
Member Author

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/&#38;&#34;<>\'/"

So only the & and the " were escaped.

@delucis delucis merged commit a37d76a into main May 3, 2024
14 checks passed
@delucis delucis deleted the chris/web-vitals branch May 3, 2024 15:40
@astrobot-houston astrobot-houston mentioned this pull request May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants