-
Notifications
You must be signed in to change notification settings - Fork 1
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
Configure context-based env vars #33
Conversation
✔️ Deploy Preview for dtp-stat-on-nextjs ready! 🔨 Explore the source changes: ef0dc0a 🔍 Inspect the deploy log: https://app.netlify.com/sites/dtp-stat-on-nextjs/deploys/61eddd085c736a000875e8c9 😎 Browse the preview: https://deploy-preview-33--dtp-stat-on-nextjs.netlify.app/ |
next.config.mjs
Outdated
*/ | ||
|
||
const suffixOrPrefix = | ||
process.env.CONTEXT === "production" |
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.
nit: consider using a hashmap to avoid double ternaries
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.
Not sure if this improves a lot in our specific scenario, but yeah, why not? 🙂 Thanks for your feedback!
|
||
if (suffixOrPrefix) { | ||
for (const key in process.env) { | ||
if (key.startsWith(`${suffixOrPrefix}_`)) { |
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.
Curious why do we need two ways of doing the same thing?
Can we just agree on prefix?
That would simplify the code, and make the naming more predicatble
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.
Yeah I agree with you that it’s best to support just one option. I’m still not sure which one will work best, so I’d like us to have some initial freedom. Netlify orders env vars alphabetically in their UI and I’m not sure whether prefix or suffix will be easier to use. My intuition says suffix, but netlify-plugin-contextual-env uses prefix.
Once we’ve experimented and established the preferred, we can simplify this code a little.
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.
I'm good with suffix, let's find agreement on this.
Netlify UI does not support context-based environment variables. For example, we cannot configure
XYZ=42
for preview releases (pull requests) andXYZ=4242
for production (main branch).As a workaround, we read Netlify’s
CONTEXT
env variable and then mapPROD_XYZ
/XYZ_PROD
orPR_XYZ
/XYZ_PR
intoXYZ
.next.config.js
runs before the rest of the app code, so our workaround applies everywhere.Prior art
https://www.npmjs.com/package/netlify-plugin-contextual-env
https://answers.netlify.com/t/environment-variable-contexts-not-working/45701/3