deno_std icon indicating copy to clipboard operation
deno_std copied to clipboard

suggestion: simplify `load()` from `std/dotenv`

Open iuioiua opened this issue 1 year ago • 5 comments

The design of load() from std/dotenv has been a source of pain for some users (see references). It does much more than it should, such as checking for required environment variables and defining default values. #4019 explored the option of deprecating this API in favor of the new --env CLI flag. IMO, --env is undoubtedly superior to load() for a few reasons. However, there may be cases when a user wants to load a .env file programmatically.

I suggest making the following breaking changes to load() in std/dotenv@1:

  1. Emulate the behavior of --env.
  2. Remove the .env.default and .env.example options.
  3. Throw only when the given .env file exists but is invalid.

References:

  • #3604
  • #3874
  • #3561
  • #3555
  • #2490
  • #3516

iuioiua avatar Apr 12 '24 05:04 iuioiua

IMO, --env is undoubtedly superior to load() for a few reasons.

Can you elaborate on this? --env is still unstable feature, and I don't see much adoption of it. It's still possible that --env could be changed drastically or removed in the future.

Emulate the behavior of --env.

Can you elaborate on this? What's the main differences?

Simplify its behavior to load a .env file given by a path, and remove all options.

Why remove all options? I saw some complaints about handling of .env.example and .env.default (#3516), but didn't see problems with others.

Return true when the given .env is loaded and false if not found.

Sounds like unnecessary breaking change to me.

kt3k avatar Apr 12 '24 06:04 kt3k

Can you elaborate on this? --env is still unstable feature, and I don't see much adoption of it. It's still possible that --env could be changed drastically or removed in the future.

Some aspects of load() have surprising behavior. E.g. see #3561. I feel like I've elaborated on load()'s design a sufficient amount here and in the issues/PRs linked above. The amount of adoption of --env is less obvious than load() as it doesn't live in code. --env can live in deno.json tasks, but even that only shows a part of the use of --env.

Can you elaborate on this? What's the main differences?

Mostly, not throwing when a .env is not found. Though, I'm unsure that doing so is the right way to go.

Why remove all options? I saw some complaints about handling of .env.example and .env.default (#3516), but didn't see problems with others.

It doesn't have to go that far. Perhaps removing .env.example and .env.default functionality is sufficient.

Sounds like unnecessary breaking change to me.

It provides feedback on whether a .env file is detected and used. AFAIK, load() currently provides no such feedback.

iuioiua avatar Apr 12 '24 08:04 iuioiua

I agree with items 1 and 4.


  1. Simplify its behavior to load a .env file given by a path, and remove all options.

If we can't reach consensus about this, we could instead keep all options, just remove their defaults ".env.example" and ".env.defaults" so that it's mandatory to specify the filenames if you want to load them.

  1. Return true when the given .env is loaded and false if not found.

Currently it returns the parsed object, which is useful. If we make that change, it will be necessary to call load() and parse() in order to know what was parsed.

One of the reasons why --env adoption could be low is that it's still pretty new, while Dotenv has years of history and many apps were already using Dotenv.

Leokuma avatar Apr 12 '24 22:04 Leokuma

Ok. I've amended by suggestion. PTAL @Leokuma and @kt3k.

iuioiua avatar Apr 14 '24 23:04 iuioiua

2. sounds good to me.

I still don't get 1. and 3. well.

Can you elaborate on this? What's the main differences?

Mostly, not throwing when a .env is not found. Though, I'm unsure that doing so is the right way to go.

To my understanding --env and load() both don't throw when the .env file not found (but the former warns if it doesn't find it).

Throw only when the given .env file exists but is invalid.

In what cases load stops throwing with this suggestion?

kt3k avatar Apr 16 '24 08:04 kt3k

Do we remove .env.example and .env.default handling before the stabilization?

related #5018

kt3k avatar Jul 03 '24 05:07 kt3k

Closing in favor of #5449 and #5464, where I suggest removing the examplePath and defaultPath options, respectively, from @std/dotenv/load.

iuioiua avatar Jul 17 '24 07:07 iuioiua