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

Revert vercel env fix #6776

Merged
merged 2 commits into from
Apr 10, 2023
Merged

Revert vercel env fix #6776

merged 2 commits into from
Apr 10, 2023

Conversation

nblackburn
Copy link
Contributor

Apologies, 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.

Changes

Testing

Didn't have the desired effect, this simply reverts it back to how it was.

Docs

  • Fixes a previously unfixed patch (specifically in the case where it was working before)
  • Remains broken for those it was broken for before

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2023

🦋 Changeset detected

Latest commit: 58d392d

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 6, 2023
@bluwy
Copy link
Member

bluwy commented Apr 7, 2023

Ah right I forgot that private env vars are only replaced when transforming files in SSR, which the analytics script is client only. As a fix, the Vercel adapter could update the Vite config to add VERCEL_ANALYTICS_ID to the envPrefix array. That should do the trick, considering VERCEL_ANALYTICS_ID is public.

@matthewp
Copy link
Contributor

matthewp commented Apr 7, 2023

@bluwy that's a fix for the initial problem, right? I'm going to assume that we need to merge this first.

@bluwy
Copy link
Member

bluwy commented Apr 7, 2023

The envPrefix trick would fix this regression, keeping the (import.meta as any).env.VERCEL_ANALYTICS_ID; part. But if we can't get around the fix soon, merging this as a stop gap sounds fine to me too.

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.

Let's merge this for now then, and we can revisit this again.

@bluwy bluwy merged commit 84a4648 into withastro:main Apr 10, 2023
@astrobot-houston astrobot-houston mentioned this pull request Apr 10, 2023
@mathieuhasum
Copy link

Maybe we could simply make the variable public by passing it to the client side.

As a workaround/POC, this works by editing the .env file in the Astro project:

PUBLIC_VERCEL_ANALYTICS_ID=${VERCEL_ANALYTICS_ID}

So maybe it would also be achievable directly in the adapter?


Otherwise, maybe the "define" could also work for this case. The Vite documentation states:

If you would like to expose an unprefixed variable, you can use define to expose it:

define: {
  'import.meta.env.ENV_VARIABLE': JSON.stringify(process.env.ENV_VARIABLE)
}

This is simply food for though, as I assume adding a envPrefix is also a perfectly viable solution 👍

@bluwy
Copy link
Member

bluwy commented Apr 11, 2023

@delucis mentioned yesterday that Vercel seems to also set the PUBLIC_ env vars on build: https://vercel.com/docs/concepts/projects/environment-variables/system-environment-variables#framework-environment-variables. So this seems to be a non-issue for now.

The docs doesn't mention PUBLIC_VERCEL_ANALYTICS_ID though, but I'd assume it also would. Maybe if it's reproducible that it isn't working, we could check with Vercel if it's correctly set.

@onebarloop
Copy link

onebarloop commented Apr 11, 2023

Otherwise, maybe the "define" could also work for this case. The Vite documentation states:

If you would like to expose an unprefixed variable, you can use define to expose it:

define: {
  'import.meta.env.ENV_VARIABLE': JSON.stringify(process.env.ENV_VARIABLE)
}

I can confirm vite-define works for this case. In astro.config:

  vite: {
    define: {
      'import.meta.env.PUBLIC_VERCEL_ANALYTICS_ID': JSON.stringify(
        process.env.VERCEL_ANALYTICS_ID
      ),
    },
  },

@nblackburn nblackburn deleted the revert-vercel-env-fix branch April 11, 2023 19:48
@nblackburn nblackburn mentioned this pull request Apr 19, 2023
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.

5 participants