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

Should environment variable values be normalized? #17

Open
CanadaHonk opened this issue Feb 11, 2024 · 5 comments
Open

Should environment variable values be normalized? #17

CanadaHonk opened this issue Feb 11, 2024 · 5 comments
Labels
agenda+ Should be discussed next meeting spec

Comments

@CanadaHonk
Copy link
Member

CanadaHonk commented Feb 11, 2024

Eg: '0' -> 0, 'false' -> false. Suggested by @nicolo-ribaudo

@CanadaHonk CanadaHonk added the agenda+ Should be discussed next meeting label Mar 7, 2024
@pi0
Copy link

pi0 commented Mar 7, 2024

Hi. Sharing some experience for doing this. For Nuxt users, we had been doing env value deserialization for couple of years using unjs/destr and it gave both positive and negative feedbacks.

I think for boolean type true|false, it would probably make sense.

Re numbers we had several user issues where tokens (intended to be string) looking like a number are wongly parsed. For example should an env with value 9100000000000001 should be parsed as number or string? (JSON.parse('9100000000000001') === 9100000000000000)

@ljharb
Copy link
Member

ljharb commented Mar 7, 2024

From my experience maintaining npmjs.com/qs, as well as working with jquery's .data() API over decades, I think implicit deserialization is a nightmarish rat's nest of footguns, and it would be far safer and less surprising to have env vars only ever be strings when present.

@nicolo-ribaudo
Copy link

nicolo-ribaudo commented Mar 13, 2024

Just spent one hour trying to figure out why some code was working locally, and failing on our CI -- it turns out that something was setting an env var to "false" 🙃 babel/babel@b1ec607

When it comes to booleans, it's incredibly annoying because there is no easy conversion from a string to boolean: Boolean(x) will convert the string "false" to the boolean true, and JSON.parse(x) will throw when the env var is not defined.

If we want to avoid implicit deserialization, it could be explicit with an API like env(name, type):

const str = env("FOO");
const str2 = env("FOO", "string");
const bool = env("FOO", "boolean");
const num = env("FOO", "number");
const int = env("FOO", "bigint");

where the type definition would be

function env(name: string): string | undefined;
function env(name: string, type: "string"): string | undefined;
function env(name: string, type: "boolean"): boolean | undefined;
function env(name: string, type: "number"): number | undefined;
function env(name: string, type: "bigint"): bigint | undefined;
function env(name: string, type: unknown): string | boolean | number | bigint | undefined;

@pi0
Copy link

pi0 commented Mar 13, 2024

Small suggestion re future compatibility if env API adds more extensions maybe it could be like this:

env(name, { type: "boolean" })

@CanadaHonk
Copy link
Member Author

Fwiw to note for future, I don't think we should worry about the API having an argument for a default value, instead allowing the user to do (... ?? defaultValue)/etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ Should be discussed next meeting spec
Projects
None yet
Development

No branches or pull requests

4 participants