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

feat: Deprecate Manual Declarations of clientEnv and serverEnv #1069

Closed
JacobADevore opened this issue Jan 10, 2023 · 20 comments · Fixed by #1071
Closed

feat: Deprecate Manual Declarations of clientEnv and serverEnv #1069

JacobADevore opened this issue Jan 10, 2023 · 20 comments · Fixed by #1071
Labels
🌟 enhancement New feature or request

Comments

@JacobADevore
Copy link
Contributor

Is your feature request related to a problem? Please describe.

The current env folder structure requires manual declarations of client environment variables which is a duplicate of the client schema. An abstraction should be made to eliminate these manual declarations.

Describe the solution you'd like to see

On client.mjs automatically create the client environment variables object by looping through the client schema.

/** @type {{ [key: string]: string | undefined; }} */ let clientEnv = {};

Object.keys(clientSchema.shape).forEach( (key) => (clientEnv[key] = process.env[key]) );

Describe alternate solutions

Leaving as is...

Additional information

No response

@JacobADevore JacobADevore added the 🌟 enhancement New feature or request label Jan 10, 2023
@juliusmarminge
Copy link
Member

juliusmarminge commented Jan 11, 2023

Can you access the env through variable indices?

I.e does

const key = Math.random() > .5 ? "FOO" : "BAR";
const value = process.env[key]
//    ^? ???

grab the correct value or will it always be undefined?

If it works that'd be great 👍

@c-ehrlich
Copy link
Member

c-ehrlich commented Jan 11, 2023

image

inference is working but initial validation seems broken - i think it's related to this line: /** @type {{ [key: string]: string | undefined; }} */ let clientEnv = {};.

i would love it if we can get this to work, would be a huge upgrade!

(edit: already changed discord secret lol)

@juliusmarminge
Copy link
Member

inference is working but initial validation seems broken

You need NEXT_PUBLIC_BAR: z.coerce.number() since it will always be a string when read from process.env

@JacobADevore
Copy link
Contributor Author

Can you access the env through variable indices?

I.e does

const key = Math.random() > .5 ? "FOO" : "BAR";
const value = process.env[key]
//    ^? ???

grab the correct value or will it always be undefined?

If it works that'd be great 👍

You can, and it does get the correct values.

@c-ehrlich
Copy link
Member

inference is working but initial validation seems broken

You need NEXT_PUBLIC_BAR: z.coerce.number() since it will always be a string when read from process.env

Wow I didn't know that, guess I've never used a number as an env variable before. Will think about a good way to add this to the docs.

image

It seems to me like everything works. @JacobADevore please feel free to open a PR for this :) Unless there's some problem I'm missing, this is a really nice dx win.

@MartelliEnrico
Copy link

Should we do the same with serverEnv and serverSchema? They recently got aligned to their client equivalent.

@JacobADevore
Copy link
Contributor Author

JacobADevore commented Jan 11, 2023

Should we do the same with serverEnv and serverSchema? They recently got aligned to their client equivalent.

Done.

@JacobADevore JacobADevore changed the title feat: Deprecate Manual Declarations of clientEnv feat: Deprecate Manual Declarations of clientEnv and serverEnv Jan 11, 2023
@danteissaias
Copy link

This works for me on the server-side but not on the client:

import { env } from "../env/client.mjs";

console.log(env);

Throws the following error on the client:

next-dev.js?c87b:20 ❌ Invalid environment variables:
 NEXT_PUBLIC_URL: Required
 NEXT_PUBLIC_STRIPE_PK: Required
 NEXT_PUBLIC_SANITY_PROJECT_ID: Required
 NEXT_PUBLIC_SANITY_DATASET: Required
 NEXT_PUBLIC_GA_MEASUREMENT_ID: Required

@c-ehrlich c-ehrlich mentioned this issue Jan 15, 2023
3 tasks
@c-ehrlich
Copy link
Member

This works for me on the server-side but not on the client:

import { env } from "../env/client.mjs";

console.log(env);

Throws the following error on the client:

next-dev.js?c87b:20 ❌ Invalid environment variables:
 NEXT_PUBLIC_URL: Required
 NEXT_PUBLIC_STRIPE_PK: Required
 NEXT_PUBLIC_SANITY_PROJECT_ID: Required
 NEXT_PUBLIC_SANITY_DATASET: Required
 NEXT_PUBLIC_GA_MEASUREMENT_ID: Required

same for me

@iduuck
Copy link
Contributor

iduuck commented Jan 15, 2023

The problem here is that Next.js (or better said the compiler) is replacing explicit usage of process.env.SOMETHING with it's respective values. Accessing through an dynamic accessor isn't going to work. So environment variables are declared on build time not runtime (like on server).

Though the new env format was introduced since the server variables are also able to be used for middleware (middleware is compiled the same as the front end files though).

@JacobADevore
Copy link
Contributor Author

What about this implementation? https://github.com/JacobADevore/next-validenv

@iduuck
Copy link
Contributor

iduuck commented Jan 16, 2023

That is the same implementation which is not going to work properly for every case in Next.js


Edit: I think i was wrong and too fast with this comment - this should work.

@juliusmarminge
Copy link
Member

@iduuck what do you think of something like this?

https://github.com/juliusmarminge/t3-env/blob/main/src/env.mjs

Only requires a single env file and a single destructed object

@iduuck
Copy link
Contributor

iduuck commented Jan 17, 2023

@juliusmarminge That's an awesome way, however, with this solution, we would have the same object type for client and server. And no linter or anything to tell us, that the server variables are not available on the client.

@juliusmarminge
Copy link
Member

juliusmarminge commented Jan 17, 2023

@juliusmarminge That's an awesome way, however, with this solution, we would have the same object type for client and server. And no linter or anything to tell us, that the server variables are not available on the client.

Yea I know - and that was sort of the goal, minimising the amount of different objects that the users have to think about. Although I get that we're now "lying" that the types are defined on the client even for server-vars and the app won't crash, just return undefined.

This is definitely a tradeoff and I'm not sure what's the best...

I can see some ways to solve this (e.g. throw an error if you access vars not prefixed NEXT_PUBLIC if window is defined) but I think that would live better in a library than userland

@iduuck
Copy link
Contributor

iduuck commented Jan 17, 2023

My personal opinion on this is, that I would like to see more "explicitness" instead of "implicitness", but then also a little blindness of the user, when and where to use the variable. And where the user can use the variable.

@juliusmarminge
Copy link
Member

juliusmarminge commented Jan 17, 2023

My personal opinion on this is, that I would like to see more "explicitness" instead of "implicitness", but then also a little blindness of the user, when and where to use the variable. And where the user can use the variable.

I'm thinking that we'd need to modify the object and the get() method in some way and check the name of the var and the typeof window and throw accordingly. You think this is possible? could use proxy somehow

@iduuck
Copy link
Contributor

iduuck commented Jan 17, 2023

HAHA, that's awesome, just finished a proxy implementation. on my fork: https://github.com/iduuck/t3-env.git

Please not that this is a very rough draft, also with many type-errors, but you get the point.

@juliusmarminge
Copy link
Member

HAHA, that's awesome, just finished a proxy implementation. on my fork: https://github.com/iduuck/t3-env.git

Please not that this is a very rough draft, also with many type-errors, but you get the point.

We're definetely on to something here though! Will take a look a bit later

@juliusmarminge
Copy link
Member

Simplified it a bit. I don't think we need to run each access through the schema since that should already be done? Just a simple server-check should do it?

https://github.com/juliusmarminge/t3-env/blob/main/src/env.mjs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants