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

@astrojs/vercel: Fix vercel analytics id not being provided #6751

Merged
merged 2 commits into from
Apr 5, 2023
Merged

@astrojs/vercel: Fix vercel analytics id not being provided #6751

merged 2 commits into from
Apr 5, 2023

Conversation

nblackburn
Copy link
Contributor

@nblackburn nblackburn commented Apr 4, 2023

While testing out vercel analytics using the vercel adapter, I noticed there was an error in my console stating that VERCEL_ANALYTICS_ID has not been provided.

After some research it looks as though it is looking for PUBLIC_VERCEL_ANALYTICS_ID but it is never provided. I believe this is likely the cause of the error.

packages/integrations/vercel/src/analytics.ts

Running vc pull with the analytics enabled downloads an environment locally which contains VERCEL_ANALYTICS_ID and not PUBLIC_VERCEL_ANALYTICS_ID. The documentation also mentions that the environment variable is inlined at build time.

https://vercel.com/docs/concepts/analytics/api#getting-started

To use the Web Vitals API, you'll need to retrieve the analytics ID for your Vercel project. This value is exposed during the build and can be accessed by process.env.VERCEL_ANALYTICS_ID inside Node.js.

Changes

  • Fix wrong environment variable being used
  • Added changeset

Testing

  • Added the missing environment variable to my build and redeployed, and it worked.
  • Ran test suite and one test fails, which I believe falls outside the scope of this change.
  • I have kept the builds up which demonstrate the workaround I put together before making any changes.

Before fix: https://welcome-to-astro-7lrfynd8t-nblackburn.vercel.app/
After fix: https://welcome-to-astro-irzdnom89-nblackburn.vercel.app/

before-fix
during-fix
not-working
post-fix

Docs

With this being, an internal change, I don't believe it requires any changes to the documentation.

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Apr 4, 2023

🦋 Changeset detected

Latest commit: 4987702

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 4, 2023
@nblackburn nblackburn changed the title Fix vercel analytics id not being provided @astrojs/vercel: Fix vercel analytics id not being provided Apr 4, 2023
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.

This make sense to me, and thanks for extensively testing it too. Don't think it would be a breaking change either as VERCEL_ANALYTICS_ID would be set by default as you mentioned.

@nblackburn
Copy link
Contributor Author

@bluwy Exactly, thanks for the review 👍

@matthewp matthewp merged commit 26daba8 into withastro:main Apr 5, 2023
@astrobot-houston astrobot-houston mentioned this pull request Apr 5, 2023
@nblackburn nblackburn deleted the fix-vercel-analytics branch April 5, 2023 13:32
@tonyski
Copy link

tonyski commented Apr 6, 2023

@nblackburn Hi, I updated this change, and i can log process.env.VERCEL_ANALYTICS_ID value.
but the site still shows: [Analytics] VERCEL_ANALYTICS_ID not found.
what is the problem?
Example link: https://astro-3btbig79z-naddod-kaka.vercel.app

@nblackburn
Copy link
Contributor Author

@tonyski In my efforts to find out why the key wasn't being included, allowed myself to lose track of the fact that only environment variables that are prefixed with PUBLIC_ will be bundled at build time. I will put a change in to revert it shortly 👍

At least for me, that environment variable isn't being provided unless I specifically set it myself.

@nblackburn nblackburn mentioned this pull request Apr 6, 2023
@nblackburn nblackburn mentioned this pull request Apr 19, 2023
@natemoo-re natemoo-re mentioned this pull request Jul 5, 2023
1 task
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.

4 participants