Removing .ok() and a new API

[sonro]

This issue(#37) and its PR(#38), highlight a pretty big problem with the API and its documentation.

The Problem

Most of the public functions return a Result with a potential error. The primary reason for an error is if a .env file is not found. Some users would like to know if their file isn't loaded, whereas some would prefer the error to be silent.

Dotenv files were conceived as a development tool to simulate a production environment. Once an app is a deployed, it would get its environment variables from its environment, not a .env file. Therefore, it is appropriate to ignore any errors.

A user is expected to ignore the error themselves with .ok(). This transforms the Result into an Option which doesn't produce any compiler warnings or errors. However, silencing errors is not the intended use-case for .ok(), which leads to potential confusion over its use for this library. Furthermore, its naming doesn't help - what does it mean to "ok" a "dotenv".

Solutions

1. Unwrap or ?

   This has already been done in a lot of the rustdoc. The error could be unwrapped, which causes a panic when the .env file is not found. Even when using the more idiomatic ? operator, the issue remains - what if the user considers their dotenv file optional?

2. New Trait

   We could add a trait which takes a Result as input and simply ignores it. The resulting API usage would look something like:

   use dotenvy::{dotenv, SilentError}
   
   fn main() {
       dotenv().silent();
       // ...
   }

   This improves the naming issue because the reason for the .silent() call is explicit. Also, those who want to handle the error, need not use the trait. However, it could be considered rather clunky - with an additional import and a bloated trait on Result.

3. Change Result to Option

   The .ok() method does this already. We could cut out this step and just return an option instead. That way the user could still handle the event of no .env file, and use the return value (the file path) if they wish. Example:

   use dotenvy::dotenv;
   
   fn main() {
       dotenv();
       // ...
   }

   Obviously this is a BREAKING CHANGE and is potentially confusing. A user who MUST have a dotenv file would probably consider "file not found" to be an error. Getting None is less informative. Also this blocks other errors in parsing the file bubbling up to the user.

4. New Functions

   By introducing a new "silent" version of the dotenv function, we could avoid breaking changes and additional imports. The silent version could return an Option, like the solution above, or it could return nothing - as the user could still use the original dotenv for error handing. API example:

   use dotenvy::dotenv_silent;
   
   fn main() {
       dotenv_silent();
       // ...
   }

   Similar to the current solution (.ok()), a user who wants to handle the error in development, but not in production, would still have to change their code or have two versions based on debug_assertions or Cargo features.

   This also fractures the code base. Other public functions would need an associated silent version too. Perhaps moving them into a separate module would improve the "feel" of the API. Moreover, the module could be hidden behind a Cargo feature flag. Module example:

   use dotenvy::silent::dotenv;
   
   fn main() {
       dotenv();
       // ...
   }

5. Documentation

   We could just improve the documentation to show how to handle AND how to ignore the errors.

Conclusion

The error handling story is tricky to deal with as there are several use cases for the library:

• Ignoring errors
• Handling errors / panicking on error
• Making use of the returned dotenv file path
• All of the above depending on Dev/Prod

It would be good if we could figure out how to document the library without alienating any of these users. The current .unwrap() solution seems like it caters to only one kind of user.

[sonro]

In all of the solutions the naming could be changed, for example dotenv_silent() could be dotenv_ignore_errors(). I've just used silent as a placeholder for an API change.

[sonro]

Personally, I prefer the 4th solution - espeically with a module. A further example:

/// If testing or not compiled with `--release`: no `.env` file causes panic.
fn init() {
    if cfg!(any(test, debug_assertions)) {
        dotenvy::dotenv().expect("can't load .env file");
    } else {
        dotenvy::silent::dotenv();
    }
}

fn main() {
    init();
}

[allan2]

Thank you for the writeup. I've had similar thoughts on confusion caused by ok(), unwrap(), and dotenv(), but I haven't had the chance to get around to it yet.

I think what you brought up is caused by shortcomings in the documentation. I want to clarify that the use of ok() in the present examples is a shortcoming. It does not imply that users are supposed to ignore errors.

ok() and unwrap() in the docs have both been sources of confusion, not helped by the lack of surrounding comments or context. I think they can reasonably be replaced with expect().

Another source of confusion is in the naming of the dotenv() function. It's not immediately clear in what it does. I'm in favour of renaming it to something more indicative of what it does, like load().

Combine these two sources together and we have a perfect storm of confusion. Like you said, what does it mean to ok a dotenv?

This is how I would replace dotenv.ok():

dotenvy::load().expect("Failed to load .env file"); 

It should be clear that load() appears to load a file, it can potentially fail, and and that it returns a Result.
The user is free to process a potential success or error however they want in typical Rust fashion.

//

A distinction between the behaviour in dev and prod modes is an interesting idea.

Although the general sense is that dotenv is for dev environments, there are many who do use .env files in production without issue. I would be hesitant to associate debug builds with dev and release builds with prod.

Dotenvy is a small library with the capability to load environment variables from a text file. My feeling is that ignoring errors, and error handling in general, should be left to the user. I think this can be assisted by including more examples.

This one incorporates the suggestions in your conclusion:

use std::str::FromStr;

enum AppEnvironment {
    Development,
    Production,
}

impl FromStr for AppEnvironment {
    type Err = String;

    fn from_str(s: &str) -> Result<Self, Self::Err> {
        match s {
            "development" => Ok(AppEnvironment::Development),
            "production" => Ok(AppEnvironment::Production),
            _ => Err(format!("Unknown app environment: {}", s)),
        }
    }
}

fn main() {
    // Loads the dotenv file
    let dotenv_path = dotenvy::dotenv().ok();

    // Loads the environment variable. It doesn't matter how how it was set.
    let app_env = dotenvy::var("APP_ENV")
        .expect("APP_ENV not found")
        .parse()
        .expect("Failed to parse APP_ENV");

    match (app_env, dotenv_path) {
        (AppEnvironment::Development, Some(path)) => {
            println!("Loaded dotenv file: {}", path.as_path().display())
        }
        (AppEnvironment::Development, None) => panic!("No .env file found"),
        (AppEnvironment::Production, Some(_)) => panic!(".env file found in production"),
        (AppEnvironment::Production, None) => (),
    }

    // proceed...
}

Thanks again for starting this discussion.

[sonro]

I would be hesitant to associate debug builds with dev and release builds with prod.

I agree: nothing like this should be mandated. I personally use this method, and only suggest that it be added as an additional example (in the examples folder) -- with descriptive commenting. I also like your example of using an "APP_ENV" environment variable to achieve a similar effect.

Dotenvy is a small library with the capability to load environment variables from a text file. My feeling is that ignoring errors, and error handling in general, should be left to the user.

You're right. There is no need for an additional API to ignore errors in a small library. However, the only ways I can find to ignore a Result is to convert it into an Option -- dotenv().ok(), or use let _ = dotenv(). The usage of which, in previous examples, is what prompted the initial confusion leading to this discussion.

To be clearer: a lot of users will be ignoring the Result of calling dotenv(), therefore I feel we should be including that use case in our documentation. Using expect or ? is a great way of quickly showing how to handle the error, but we should also show how to ignore it.

I want to clarify that the use of ok() in the present examples is a shortcoming.

As I stated, its naming is a big problem, but we should show a way to ignore the Result. These users only want to load a .env file if it is present. This comment sums up the frustration with using the library in this manner. Plus the one above it, in the PR discussion, does display that .ok "makes it clear enough that failure is acceptable", at least for some users. So if we are going to drop any mention of .ok in our documentation, we should provide an alternative - hence my proposed API changes.

A simpler solution would be to add more examples (including in the readme), some of which ignore the errors, others handle them.

[sonro]

I also prefer using load over dotenv.

dotenvy::load().expect("Failed to load .env file"); 

However, as dotenv is such a fundamental function for this library, I would suggest a slow transition to load. Perhaps a few months before the old function is even deprecated, and keeping it around for a long time afterwards.

[kaj]

As I (kind of) mention in #44 , I think a .silent() (or dotenv_silent()) method is problematic. Or rather, that it implies that it silences all errors, which I think is problematic; if there is a .env file but it contains an error or can't be read by the current user, I would want to know it.

So what I'm suggesting there is to change the return type of dotenv() to Result<Option<PathBuf>>. Then users like me, who thinks not having a .env file is ok but failing to read an existing one is an error can use:

   dotenv()?; // or .unvrap() if not returning a result

while users who require a .env file can use:

  dotenv()?.ok_or(MyError::MissingDotenv)?;

We could of course also add an extension trait to Result<Option<PathBuf>> so code like this would give the current missing-is-error behaviour, but that would require an extra import.

  dotenv().required()?;

[sonro]

I like this change to the API. It can elegantly handle all use cases. I feel the extra import of a trait is unnecessary for such a small library.

As this would be a BREAKING CHANGE, I think it should be bundled with the renaming of dotenv to load as per @allan2's idea.

[sonro]

@allan2 it would be good to know your thoughts on this change.

[allan2]

I'm still considering the suggestions... I'd like to look into it more and consider how other dotenv libraries handle this.

Expect a reply in the coming days :)

[FrankTaylorLieder]

Hi. I've started using this library for a few personal projects. I too agree that we should make .env being optional easy.

To that end I forked the project and added a new loader: dotenvy::dotenv_optional(). This is basically a non-breaking version of @kaj 's proposal above.

See here.

If the aim of this project is to be a drop-in replacement for the original dotenv, then we should be not break the existing API. Adding new calls is the way to go in the short term.

That said, a root and branch redesign of the API would be welcome.

LMK if you'd like me to prepare a pull request for the changes above. I am happy to make adjustments, e.g. to naming.

[FrankTaylorLieder]

Just to follow up, I've built a builder version of the API. Take a look at: https://github.com/FrankTaylorLieder/dotenvy/tree/20240319-builder

It was quite some wrestling with Rust over types, but I've got something coherent. I'd be interested in feedback. I'm not totally convinced its the best solution... there may be a simpler approach which yields the same utility.

This approach reduces the number of API methods from ~20 for all combinations of source, optionality, override, load/iterator to just 8. Examples...

dotenvy::builder::dotenv().load();
dotenvy::builder::from_filename(".env.production").allow_missing().override_duplicates().load();
dotenvy::builder::from_read(&mut File::open("/foo/bar").unwrap()).iter();

One other thing. I was having problems running the tests... they seemed to be colliding as they all concurrently referenced the shared process environment. This is probably because I've been putting multiple tests into the same source file. So I've added some synchronisation around the tests and setting/cleaning the env.

Just looking for feedback at the moment... are you interested in going in this direction?

[allan2]

My thoughts:

A builder pattern or config pattern is useful when there is more to be done. dotenv isn't that complicated though. A single call of load or load_opt suffices for most programs. override variants exist for those who need it. I'm in favour of keeping the API in its current form.

[allan2]

The next question is whether we need _opt versions of the other calls - from_filename, from_path, *_override, *_iter.

I think load_opt is enough. No need for others.

Sorry for my late reply @FrankTaylorLieder. I could've saved you some effort. I do appreciate your proposal though.

[FrankTaylorLieder]

(Don't worry about time spent... it was a useful learning exercise to build, which is why I just did it.)

I mostly agree, the builder does look like overkill for such a small number of options. But there is complexity hiding...

load_opt() only works for the default ".env" and only for non-override cases, and not for iter() results. What about other cases? Loading from filename or path, with overrides, getting an iter?

Assuming we want full flexibility for the API, we'll need to implement all combinations of source, options and output. As such we'll need 21 functions in the API. (See below for working. Today the API has 12 functions.)

The builder has just 8 functions in the API to cover all cases.

Looking at node.js dotenv, they use a config to specify options. We could look at this, but under the hood it ends up being similar code. Indeed the builder (using a typestate pattern) allows use to restrict invalid combinations at compile time. E.g. from_read(r).allow_missing().load() is not allowed as it does not make sense for readers.

Whilst we can just go with load_opt() for now, I suspect requests for the full set of combinations will come (probably from me :-) ).

---

Combinations:
3 - 3 file sources (default, filename, path)
6 - x2 as each source be optional
7 - +1 as readers are a valid source, but can't be optional
14 - x2 as all sources can specify override - this is the number of load() options
21 - +7 as iter() variants are needed for each source/optional combination (not for override)

[aidenfarley]

I personally like the builder design much more than any of the other options. I'm confused as to your reasoning against it @allan2. Frank states the increasingly larger number of combinations required to cater to the various audiences using dotenvy. In my eyes, dotenvy is complicated enough to warrant a builder.

What reasons do we have to go against a builder design? If it's breaking API changes, just keep the behavior the same by default, and in some future release change the default behavior if warranted. The builder gives us an easily readable, verbose design when using dotenvy.

[allan2]

Below, I refer to dotenvy::dotenv as load and the proposed function with Result<Option<PathBuf>> signature as load_opt.

let _: PathBuf = load()?;              // errors if file is missing or invalid
let _: Option<PathBuf> = load_opt()?;  // ignores non-existent files, errors if file is invalid

How other implementations do it

godotenv errors when the file is not found. This is like load.

// https://github.com/joho/godotenv/blob/3fc4292b58a67b78e1dbb6e47b4879a6cc602ec4/godotenv.go#L40-L61
func Load(filenames ...string) (err error) {
	filenames = filenamesOrDefault(filenames)

	for _, filename := range filenames {
		err = loadFile(filename, false)
		if err != nil {
			return // return early on a spazout
		}
	}
	return
}

Ruby has an ignore flag, which is defaults to true. The default is like load_opt.

# https://github.com/bkeepers/dotenv/blob/c9ecfa4e5598593dbadcf6c610cf5d8a9ed0bec1/lib/dotenv.rb#L17-L24
def load(*filenames, overwrite: false, ignore: true)
  parse(*filenames, overwrite: overwrite, ignore: ignore) do |env|
    instrument(:load, env: env) do |payload|
      update(env, overwrite: overwrite)
    end
  end
end

My thoughts

load_opt would be convenient for those who want to optionally load a dotenv file.

I am in favour, as long as we caution against calling dotenv loading functions in prod environments. This can be covered with documentation.

@sonro @Dylan-DPC?

[fzn0x]

As long as you don't expose your secrets during build time to public I think that's fine. Secrets was managed in runtime and dotenvy could did the same.

[FrankTaylorLieder]

It might be worth pointing off to a few better ways to handle environment in Rust in production. I'm not sure I've seen many, unless we are talking about platform specific options which are unique to a specific platform.

There are many tutorials out there that recommend using (e.g.) .env.production for production use cases.

[aidenfarley]

Maybe we consider reading from .production.env in release mode and .env for debug? I can imagine some conflict if a builder pattern were implemented for the new .load()
For example, if I have a .env file but no .production.env file, using .load().error_if_not_exist(true).unwrap() (or such a variant) will cause an error when I finally build my project in release mode had I only built debug previously.

Without explicit documentation of this behavior, I think it can be detrimental to a workflow instead of positive.

All in all, I am for having two separate files as long as it is explicitly documented that it has diverging behavior between debug and release.

[FrankTaylorLieder]

I don't think aligning release builds with production deployment is right - there could be any number of environments a release build is pushed to.

How opinionated should dotenvy be? If we are to be opinionated, then we should carefully lay out which use cases we support and how to use them. But we risk not aligning with real-world usage and the library not being applicable.

If not (my preference), then we need to be careful not to make assumptions... someone may decide that all environment is loaded from .env, but they manage the content in different deployment environments. The default file loaded should always be .env, if you want a different file then you specify it as the source. E.g. (using the builder in the thread above) from_file(".env.foo").load()

Or you could get fancy, attempting to load different files:

if from_filename(".env.production").allow_missing().load()?.is_some() {
  println!("Loaded production env");
} else if from_filename(".env.staging").allow_missing().load()?.is_some() {
  println!("Loaded staging env");
} else if dotenv().allow_missing().load()?.is_some() {
  println!("Loaded debug env");
} else {
  println!("Could not find an env file");
}

Even better, we could support an ordered loading sequence:

from_first_filename([".env.production", ".env.staging", ".env"]).allow_missing().load()?;

Further, if the user wanted a set of defaults... it could be:

from_filename(".env.defaults").load()?;  // Note: no allow_missing() option.
from_first_filename([".env.production", ".env.staging", ".env"]).allow_missing().override_duplicates().load()?;

[aidenfarley]

I agree with not aligning release with production -- I'm not sure what I was thinking when I wrote that yesterday.

First off, is the default behavior we're looking for a panic on missing file?

Second, I was under the impression all of these would be under a separate builder (ignore the way I made my previous response, I'm not sure what approach I was taking there). For example:

dotenvy::build().file(".custom.location.env").allow_missing().files([".second.env", ".third.env"]).load()
// Do we plan to have this return a Result<Option<String>> or Result<String> (with given error type of course)

Another option is to only have one method called .files() instead of a separate .file()

Third, could you clarify your intention with .allow_missing()? Does it disable a panic and return an error result instead, or does it return an error by default and return success with an empty string on failing to find a file?

[FrankTaylorLieder]

I agree with not aligning release with production -- I'm not sure what I was thinking when I wrote that yesterday.

:-)

First off, is the default behavior we're looking for a panic on missing file?

It's not a panic, just an error, and is the behaviour today. Handling this error when you want to allow the env file to be missing is tricky as you need to interpret the content of the Error::Io(io::Error) result. This discussion is about enabling a softer response to missing env files.

Second, I was under the impression all of these would be under a separate builder (ignore the way I made my previous response, I'm not sure what approach I was taking there). ...

Yes, this is part of the proposal above. Creating a builder would enable a root-and-branch update to the API to enable more flexibility. Check out a proposal here: #39 (reply in thread)

Another option is to only have one method called .files() instead of a separate .file()

Yes.

We could go one further and attach file options to a file being added to the builder. e.g.

        dotenvy::builder()
            .add(filename(".env.staging").allow_missing())
            .add(default())
            .first()
            .load()?;

This would allow a staging env to be loaded in preference to a default .env but fail if none are found.

Third, could you clarify your intention with .allow_missing()? Does it disable a panic and return an error result instead, or does it return an error by default and return success with an empty string on failing to find a file?

The idea is to ensure that an error is not returned for a missing file, instead we use Option to signal if a file was found and loaded. We can still get errors, e.g. if the file contents are malformed. (The library does not panic, instead it returns errors which the caller may handle by panicing if wanted.)