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

Expose env vars globally #2244

Merged
merged 5 commits into from
Mar 30, 2023
Merged

Expose env vars globally #2244

merged 5 commits into from
Mar 30, 2023

Conversation

notmd
Copy link
Collaborator

@notmd notmd commented Mar 27, 2023

A small downside is that we have to use getServerSideProps on every page.

@notmd notmd marked this pull request as ready for review March 28, 2023 15:03
@notmd notmd requested a review from AbdBarho as a code owner March 28, 2023 15:03
Copy link
Collaborator

@AbdBarho AbdBarho left a comment

Choose a reason for hiding this comment

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

Do you have a rough estimation of how much more additional computation required for the server-side rendering? we don't want CPU usage to sky rocket for some env variables.

Comment on lines 42 to 44
for (const key in env) {
process.env[key] = env[key];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we maybe do it like this?

process.env = new Proxy(env, {
    get(env, key) {
        const value = env[key];
        if (!value) {
           throw new Error("Environment variable not set in _app.tsx")
        }
        return value;
    }
)

I think it is very easy to make this mistake, which is why we should log how to fix it..

I hope nothing else breaks, otherwise just Object.assign(process.env, env)

ENABLE_EMAIL_SIGNIN_CAPTCHA: boolean(process.env.ENABLE_EMAIL_SIGNIN_CAPTCHA),
CLOUDFLARE_CAPTCHA_SITE_KEY: boolean(process.env.CLOUDFLARE_CAPTCHA_SITE_KEY),
},
cookie: req?.headers.cookie,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you check if this is still needed? I remember that I did some debugging, and this cookie field was always empty.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The req object only available when we use getServerSideProps. So it still works if the page is used with getServerSideProps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So now we are using mixed getStaticProps and getServerSideProps, it only partially works. I think that's fine

@notmd
Copy link
Collaborator Author

notmd commented Mar 29, 2023

Do you have a rough estimation of how much more additional computation required for the server-side rendering? we don't want CPU usage to sky rocket for some env variables.

Haven't done any benchmarks yet. But I'm running a NextJS app (quite small) that do full SSR, it doesn't use so much CPU. NextJs is really lightweight, SSR only adds extra few milliseconds to the response latency. Let me try to switch page do not use env var to static first.

@notmd
Copy link
Collaborator Author

notmd commented Mar 30, 2023

I managed to use getStaticProps. In my previous implementation, my concern is when the user navigates from static page to ssr page the process.env is not available. But look like next/router will handle all of this (it's still call the server to get the props.).

I also add a warning when we try to access env that is not available (I can't throw an error because the markdown lib also uses env for some special rendering). But this still has some caveats, if we try to access env var "statically" eg: const a = process.env.SOME_THING on a top-level scope of the module, env var won't be able at that time, and the warning is not triggered too. We have to use env var carefully on the client side.

@AbdBarho AbdBarho merged commit 85c3ea6 into main Mar 30, 2023
@AbdBarho AbdBarho deleted the expose_env_vars_globally branch March 30, 2023 17:15
dvruette pushed a commit that referenced this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants