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: std/env with assertEnvSet() and setEnvDefault() #3874

Closed
wants to merge 15 commits into from
Closed

Conversation

iuioiua
Copy link
Collaborator

@iuioiua iuioiua commented Nov 29, 2023

This PR adds a new std/env sub-module with assertEnvSet() and setEnvDefault() that, if accepted, replaces std/dotenv's load(), when used with the new --env CLI argument.

load() does quite a bit. In short, it reads and parses a .env file, ensures that it contains required environment variables as defined by a .env.example, and sets any default environment variable values as defined by a .env.defaults. Once done, it sets these environment variables in the current process. There's more to it than that, but that's the gist. Documentation here.

assertEnvSet() and setEnvDefault() perform a subset of this functionality, but in a fundamentally different and simpler way - by directly interacting with Deno.env and defining required and default environment variable information in-code instead of in .env.* files.

It allows the Deno runtime to take care of actually loading the .env file, using the --env CLI argument, and instead just checks the current processes environment variables set. In other words, these functions skip reading and parsing of .env.

Required environment variables and default environment variables are defined in-code instead of obscure .env.*. Previously, .env.* files were saved to repos anyway. This is safe to do, as these specific values (required and defined env. vars.) are not confidential.

One of the nice things about load()'s approach is that by having environment variables in .env.* files, one can copy and paste the keys to the actual .env file in the correct format. When assertEnvSet() throws because of a missing required environment variable, it also prints the error in the .env format, reducing any possible guesswork regarding the environment variable the error refers to.

I understand this differs from the dotenv convention, but I think it's a far simpler and cleaner way of ensuring environment variables are set up correctly.

If accepted in favour of load():

@iuioiua iuioiua marked this pull request as ready for review November 29, 2023 01:23
@iuioiua iuioiua requested a review from kt3k as a code owner November 29, 2023 01:23
@timreichen
Copy link
Contributor

I think the name prepare does not describe well what the function is actually doing.
If we include it in std, I think it should be more straight forward, maybe even split the functionality to something like:

import { setDefault, assert } from "https://deno.land/std@$STD_VERSION/dotenv/mod.ts";

// sets a default value in Deno.env if the key does not already exist.
setDefault("foo", "bar")

// throws an error if value does not exist in Deno.env
assert("foo")

@iuioiua
Copy link
Collaborator Author

iuioiua commented Nov 29, 2023

Yeah, fair points. I wasn't certain about the name either. I very much like the idea of splitting this up into 2 separate functions too. How about the names setEnvDefault() and assertEnvSet()?

@iuioiua iuioiua marked this pull request as draft November 29, 2023 23:31
@timreichen
Copy link
Contributor

timreichen commented Nov 29, 2023

Yeah, fair points. I wasn't certain about the name either. I very much like the idea of splitting this up into 2 separate functions too. How about the names setEnvDefault() and assertEnvSet()?

Is the env in the names necessary in a mod called dotenv? Includeing Env could be confusing because one might think it handles a whole Env object instead of a single key/value pair.

Maybe instead of assert(), assertExists() would be more consistent with std/assert/assert_exists.
I couldn't find a common name for a setIfAbsent function. Seems like every language has a different name for that. So maybe there is a better name out there instead of setDefault()...

But assuming the import is done like this:

import * as env from "https://deno.land/std@$STD_VERSION/dotenv/mod.ts";

// sets a default value in Deno.env if the key does not already exist.
env.setDefault("foo", "bar")

// throws an error if value does not exist in Deno.env
env.assertExists("foo")

seems pretty clear to me.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Nov 29, 2023

Is the env in the names necessary in a mod called dotenv? Includeing Env could be confusing because one might think it handles a whole Env object instead of a single key/value pair.

I'm thinking the same now too. These functions would probably best done in a new std/env.

Maybe instead of assert(), assertExists() would be more consistent with std/assert/assert_exists. I couldn't find a common name for a setIfAbsent function. Seems like every language has a different name for that. So maybe there is a better name out there instead of setDefault()...

But assuming the import is done like this:

import * as env from "https://deno.land/std@$STD_VERSION/dotenv/mod.ts";

// sets a default value in Deno.env if the key does not already exist.
env.setDefault("foo", "bar")

// throws an error if value does not exist in Deno.env
env.assertExists("foo")

seems pretty clear to me.

But that's not always how the functions will be imported. They may get imported from an intermediary deps.ts file.

For now, I've gone ahead with setEnvDefault() and assertEnvSet(). Let's see what others think.

@iuioiua iuioiua changed the title feat(dotenv): prepare() feat: std/env with assertEnvSet() and setEnvDefault() Nov 29, 2023
@iuioiua iuioiua marked this pull request as ready for review November 30, 2023 02:16
@timreichen
Copy link
Contributor

ref emplace at stage 2.

@kt3k
Copy link
Member

kt3k commented Dec 4, 2023

I'm -1 to this.

a new std/env sub-module with assertEnvSet() and setEnvDefault() that, if accepted, replaces std/dotenv's load().

I don't think these can replace load of dotenv. The main point of load is, as its name suggests, loading of the env variables, not validating or supplying defaults.

Also assertEnvSet and setEnvDefault both look too trivial to be included in std.

if (!Deno.env.has("FOO")) Deno.env.set("FOO", "BAR");

is clearer in my view than:

import { setEnvDefault } from "std/env/mod.ts";
setEnvDefault("FOO", "BAR")

@iuioiua
Copy link
Collaborator Author

iuioiua commented Dec 4, 2023

Apologies for the mixup. To clarify, I hope for --env, assertEnvSet() and setEnvDefault() to replace load(). I've tweaked my initial comment.

Perhaps, take a second look at load()'s documentation. It involves a defaultsPath, which defines the path of a .env file which dictates the default environment variable values for the given process. setEnvDefault() does this in a far more direct manner.

I no longer argue triviality as a reason against adding something to the Standard Library. There are plenty of examples of trivial yet very useful APIs in the Standard Library. E.g. some functions in std/assert and std/collections.

Instead, we should perhaps use usefulness (the chance that someone will need said functionality) as a metric. Seeing that people do in fact use load() for its ability to load .env files and define default and required environment variables, there is demand for this functionality.

This PR provides a more straightforward approach to satisfying these requirements. If triviality is still a concern, I was thinking of adding a passive option, which, if true, only interacts with Deno.env if the --allow-env permission is granted. E.g. they'd replace how SaaSKit and KV OAuth handle things.

@iuioiua iuioiua requested review from lucacasonato and removed request for lucacasonato December 6, 2023 02:03
@kt3k
Copy link
Member

kt3k commented Dec 11, 2023

Seeing that people do in fact use load() for its ability to load .env files and define default and required environment variables, there is demand for this functionality.

From my observation, the features of checking required and supplying defaults are not essential part of dotenv. Rather people often seem annoyed or confused by that feature enabled by default (For example, this suggests making them optional)

I don't think providing these 2 APIs help people migrating from std/dotenv to --env.

I hope for --env, assertEnvSet() and setEnvDefault() to replace load()

To my understanding of the discussion in #3604, some people are in favor of keeping to use std/dotenv because of 'automatic' discovery capability of .env file (It's specified by source code, not command line flag). Because --env doesn't have it, I don't think migration is not an option to such users.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Dec 19, 2023

PTAL @timreichen, @jsejcksn, @cknight, @andrewthauer and @Leokuma. I'd like to hear what you guys think about this.

@timreichen
Copy link
Contributor

I am unbiased about this. We also could just add a warning message in load() and loadSync() encouraging the usage of --env.

@andrewthauer
Copy link
Contributor

andrewthauer commented Dec 19, 2023

At a minimum, I would like to see both parse and stringify as part of std. I personally don't have a lot of need for the more opinionated load function tbh.

As for assertEnvSet, I would normally use direnv with env_vars_required for local development. Both assertEnvSet and setEnvDefault also cross into some other territory such as validation of the value. You might also need to parse the value into a different datatype, etc. which quickly becomes domain specific. So tbh, I'm not entirely sure if I would reach for either of these very often.

@jsejcksn
Copy link
Contributor

jsejcksn commented Dec 20, 2023

My understanding is that there is a continuum of use cases when working with plaintext files which consist of key-values pairs.

At one end: a user wants as little configuration and as much "magic" as possible — they expect the offered API to figure out where their ".env" file is (maybe some cascade of expected locations) and load the pairs into the runtime environment variable dictionary/map/etc.

At the opposite end: a user will somehow obtain a string of such data at some point during the program's lifecycle (or need to generate one!), and they need a clearly defined specification for the serialization/deserialization of the data, along with composable functional primitives to help with manually completing tasks which deal with key-value pairs and this format.

It appears that Deno's --env CLI argument wants to take on one end of the continuum (closer to the configuration-less magic), but there are many other users at each point along the rest of the continuum, and I hope that dotenv can provide usefulness to them.

For us: a clear challenge is to define exactly which combinations of primitives provide utility to users on the continuum, but I expect that the standard library will continue to offer primitives — and perhaps in the future: a de-/serialization specification so that (especially new) users can have confidence in the format of their env files without needing to fully review the source code of the module.

The spec that I'm describing would exhaustively answer questions that many users have about the format, such as:

  • Are single-quoted values allowed? How about double-quoted?
  • What about whitespace before/after a key/value?
  • What if a value includes =?
  • Are multiline values supported? What's the syntax for that?
  • What types of newlines are supported? Just LF or CRLF?
  • …etc.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Dec 21, 2023

I'm going to close this PR for now. It seems as though we need a bit more discussion.

For us: a clear challenge is to define exactly which combinations of primitives provide utility to users on the continuum, but I expect that the standard library will continue to offer primitives — and perhaps in the future: a de-/serialization specification so that (especially new) users can have confidence in the format of their env files without needing to fully review the source code of the module.

The spec that I'm describing would exhaustively answer questions that many users have about the format, such as:

  • Are single-quoted values allowed? How about double-quoted?
  • What about whitespace before/after a key/value?
  • What if a value includes =?
  • Are multiline values supported? What's the syntax for that?
  • What types of newlines are supported? Just LF or CRLF?
  • …etc.

For future reference, let's try to stay on track with the issue at hand. While this is an interesting idea, it's best discussed in a new issue 🙂

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