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

Update README.md #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update README.md #2

wants to merge 2 commits into from

Conversation

iliane5
Copy link

@iliane5 iliane5 commented Feb 19, 2024

This corrects the names of the runtime environment variables necessary to override the redis connection config option

Thanks for the great work on the package btw, it's a godsend!

Fix the runtime environment variables names
@genu
Copy link
Owner

genu commented Feb 23, 2024

Hi There,

Can you clarify the need for this change?

In:

https://github.com/genu/nuxt-concierge/blob/9bfa966bd664383138b571723a6bdacac4017178/src/module.ts#L40C3-L44C49

we're reading from NUXT_REDIS_* so changing them would incorrectly set the default values.

Unless I'm missing something?

@iliane5
Copy link
Author

iliane5 commented Feb 23, 2024

Hi!

Yeah Nuxt is a bit weird/specific with the runtime config. Here by setting the default values from NUXT_REDIS_, it'll only work in dev mode or if you run nuxt build with the NUXT_REDIS_ variables available. You can see this by building the app with your .env file and then trying to change the port or password in the .env file which won't work. You'll have to use the NUXT_CONCIERGE_REDIS_* variables to override the runtime config in production.

It's explained a bit more in detail here:

Setting the default of runtimeConfig values to differently named environment variables (for example setting myVar to process.env.OTHER_VARIABLE) will only work during build-time and will break on runtime. It is advised to use environment variables that match the structure of your runtimeConfig object.

What I would advise is using NUXT_CONCIERGE_REDIS_* everywhere instead of NUXT_REDIS_*, which is slightly longer but will work transparently for people in dev/staging/prod, etc.

@devi4nt
Copy link

devi4nt commented Apr 16, 2024

+1 for this PR, we've had to use these alternate ENV vars to get the production nuxt with concierge up and running in a docker container. If nothing else a little line in the README explaining this alternate method of defining the config, would of saved us a fair amount of troubleshooting / debugging time.

host: process.env.NUXT_REDIS_HOST,
port: Number(process.env.NUXT_REDIS_PORT),
password: process.env.NUXT_REDIS_PASSWORD,
host: process.env.NUXT_CONCIERGE_REDIS_HOST,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

including the old ENV as a fallback would allow this to be safely merged without impacting existing systems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants