-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
feat(nextjs): Default to VERCEL_ENV
as environment
#7227
Conversation
packages/nextjs/src/client/index.ts
Outdated
|
||
options.environment = | ||
options.environment || | ||
(process.env.NEXT_PUBLIC_VERCEL_ENV ? `vercel-${process.env.NEXT_PUBLIC_VERCEL_ENV}` : undefined) || |
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.
l: Can we maybe put this into a small util like:
function getVercelEnv(): string | undefined {
const vercelEnv = process.env.NEXT_PUBLIC_VERCEL_ENV;
return vercelEnv && `vercel-${vercelEnv}`;
}
Feels better to me IMHO than to sprinkle this through the rest of the code - then we can just do e.g.
options.environment = options.environment || getVercelEnv() || process.env.NODE_ENV;
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.
It would just deduplicate this once because we have VERCEL_ENV
for node and edge and NEXT_PUBLIC_VERCEL_ENV
for the client.
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.
can't we just dynamically chose via a boolean?
I would also like to see this as a func because this is totally something that where we only update two, but forgot to update one (because of merge conflicts, quick fix PR etc.)
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.
Democracy wins :p dfc5d20
size-limit report 📦
|
Replay SDK metrics 🚀
develop |
Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
---|---|---|---|---|---|---|---|---|---|
7f4c4ec | +56.64 ms | -0.00 ms | +5.57 pp | +927.42 kB | +1.06 MB | +2.21 kB | +41 B | +1 | +110.83 ms |
00d2360 | +55.18 ms | +0.00 ms | +2.23 pp | +934.14 kB | +1.05 MB | +2.22 kB | +41 B | +1 | +71.65 ms |
Last updated: Mon, 20 Feb 2023 09:46:38 GMT
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.
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.
@lforst I'm not sure I fully understand the reason behind the prefix. I'm also concerned that it will now mark new production events as BTW, you can just use |
I say the prefix is useful to discriminate between the vercel environment a build gets deployed to and the local environment where it is likely that People will have the option to override this default by setting the environment option. |
Fixes #6993
This PR is just a quick win that should improve the ootb experience for Vercel on Next.js by setting the environment option to a sensible default provided by the hosting provider.
The
vercel-
prefix is so you can discern the verceldevelopment
environment from local development because there theNODE_ENV
var withdevelopment
is likely gonna be picked up.