[@astrojs/vercel] Individually enable Speed Insights and Web Analytics #8021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 1dae36e 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
bot
added
the
pkg: integration
|
Thanks @chriswdmr! Conceptually, this change LGTM. To bikeshed on the exact API a bit, I wonder if we want to make this backwards compatible and allow I'm also a bit worried about exposing all the underlying options from |
Good point! I think it's super tricky since we can't know which features are available and enabled on the user side. Hence why I dedicated to not leave the
Oh, I didn't know that, thanks for the callout. I'm not sure how other adapters are handling that, do you have some pointers? Or is the general ideal to have some "simple" support but if you want to do more things you opt out of the adapter? |
|
@chriswdmr The reason why this won't work is that we don't build the config file as part of the production bundle (as it often contains a lot of dev dependencies). The pattern we often use instead is to allow the user to have a module that they specify some exports from. I do this in one of my own adapters, see this: https://github.com/matthewp/astro-fastify#entry So in your example you might have them define a module like export const beforeSend = (event) => {
// Ignore all events that have a `/private` inside the URL
if (event.url.includes('/private')) {
return null;
}
return event;
}And then have them pass that module as a config option. Then in your injected script import it like so: import { inject } from '@vercel/analytics';
import { beforeSend } from '${userDefinedModule}';
inject({
mode: ${config?.mode},
beforeSend: beforeSend,
debug: ${config?.debug}
});`;This is pseudo-code of course, also want to account from them not defining such module. |
ematipico
requested changes
Aug 29, 2023
There was a problem hiding this comment.
A couple of essential things we should address:
- This change removes an option without deprecating it first. This is important for us, especially for libraries that we directly maintain. We should keep
analyticsand backwards compatibility for some weeks/months. - This change doesn't provide any migration guide form the old option. We have to write this migration inside the changelog.
|
@ematipico thanks for the review!
Makes sense, I'll update the PR. Is it fine if I add a
Sorry that I missed that. Will add it to the changelog. |
|
@chriswdmr a warning would be very much appreciated for this. |
|
@matthewp @ematipico I've updated the PR based on your feedback. |
|
Thank you @chriswdmr ! We'll keep the PR open for a few days, we have a big release coming up, so we want to avoid merging big PRs unless they are important fixes. Please bear with us for a little while. |
sarah11918
reviewed
Sep 3, 2023
There was a problem hiding this comment.
Thanks for the docs contributions for this @chriswdmr ! It's quite comprehensive and well done! Just two comments from me re: the README below!
@ematipico has already give you some great docs advice, and I'm sure whatever final decisions you and he make will be awesome! 🙌
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
sarah11918
approved these changes
Sep 5, 2023
|
!preview vercel-upgrade |
|
|
!preview vercel-upgrade |
|
|
!preview vercel-upgrade |
|
|
I had to create a new PR, I think because this is from the fork's |
ematipico
approved these changes
Sep 11, 2023
bluwy
reviewed
Sep 12, 2023
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
matthewp
requested changes
Sep 12, 2023
matthewp
approved these changes
Sep 14, 2023
matthewp
approved these changes
Sep 14, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Major plugin changes
Added:
webAnalyticspropertyspeedInsightspropertyIntroduces dedicated properties on the plugin to enable Vercel Speed Insights and Vercel Web Analytics individually.
Removed:
analyticspropertyIt got removed because it toggled both individual features at once which was not ideal since not every user has both features enabled.
It also threw an error when only Web Analytics was enabled since the environment variable for Speed Insights
VERCEL_ANALYTICS_IDwas not available.Testing
I've added basic tests to verify the features are loaded correctly.
Docs
I've also updated the docs accordingly, let me know if I can make them better
/cc @withastro/maintainers-docs for feedback!