t3-oss / t3-env

https://env.t3.gg
MIT License
2.45k stars 79 forks source link

shared environment variables should enforce client prefix #209

Closed ciiqr closed 3 months ago

ciiqr commented 3 months ago

Shared environment variables require client prefix to be accessible from both the client and the server, however the client prefix is not enforced by the types, resulting in Invalid environment variables errors at runtime (when used incorrectly). With Next the only exception to this is NODE_ENV (and technically internal __NEXT_*). I'm not sure how consistent this behavior is between frameworks though (there may be other non-prefixed env to whitelist per-framework).

Currently doing to following to add some safety:

// NOTE: non-default env vars need the `NEXT_PUBLIC_` prefix to be shared
type SharedEnv = Record<"NODE_ENV" | `NEXT_PUBLIC_${string}`, unknown>;

export const env = createEnv({
    shared: {
        NODE_ENV: z.enum(["development", "test", "production"]),
        NEXT_PUBLIC_CLIENT_VAR: z.string(),
    } satisfies SharedEnv,
    ...
});
juliusmarminge commented 3 months ago

Nope - in your case, NEXT_PUBLIC_CLIENT_VAR should go under client, and NODE_ENV is a shared one. The reason is cause NODE_ENV is available on the client even without being prefixed.

There are not a lot of other variables like this so you should use it carefully. client and server should be sufficient for most. (note that server can access client vars just fine)

ciiqr commented 3 months ago

The reason is cause NODE_ENV is available on the client even without being prefixed.

There are not a lot of other variables like this so you should use it carefully.

Yes, which is why with the current interface it feels from the outside like we'd want to add exceptions for those to the shared envs, and otherwise enforce the prefix.

server can access client vars just fine

It sounds like this is maybe a communication issue with the api then. I'm not sure that client/shared even makes sense as a concepts if you're encouraging people to access client env vars on the server. Maybe if client was was called public it would be a bit clearer the intention? with the current naming, it really feels like I'd want to put "common" env in the shared section (ie. anything I want to access from both client and server).

juliusmarminge commented 3 months ago

Thats good feedback. I'll think about it. If you feel like it feel free to modify the docs to convey the current behavior better