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

Feedback on proposal: Deprecate std/dotenv #3604

Closed
cknight opened this issue Sep 1, 2023 · 14 comments
Closed

Feedback on proposal: Deprecate std/dotenv #3604

cknight opened this issue Sep 1, 2023 · 14 comments

Comments

@cknight
Copy link
Contributor

cknight commented Sep 1, 2023

#3489 (comment) proposes deprecating std/dotenv as it overlaps with the new --env CLI flag proposal. This issue has been raised to capture discussion on the topic.

@cknight
Copy link
Contributor Author

cknight commented Sep 1, 2023

My own view is that as it currently stands, --env only provides a subset of the functionality which dotenv provides, and therefore dotenv provides sufficient benefit to the user to remain a std module. Specifically, it allows:

  • a .env file to be parsed as a configuration object. This means there is no dependency on allowing env permissions. N.b. the --env proposal is considering the ability to automatically allow env permissions for env vars specified in the .env file without the need to specifically declare permissions for them, however this is still under discussion. Another use case for the configuration object is the ability to log or output all env vars in use without knowing what the individual env var keys are. This is not possible with the --env proposal, though you could output the entire process env environment with full env permission grant.
  • dotenv offers (the poorly named) .env.example file, allowing the project/module to dictate which environment variables it expects to be present in the .env file or process environment. These can also offer documentation on each expected env var. This is useful for setups where the .env file is dynamically sourced for each environment and offers auto-validation.
  • dotenv offers the .env.defaults file to supply default env values when these are missing from the .env file. Again, this is useful in environments where you are externally supplying a .env file.

Much of this functionality (except the configuration object) could be moved to the --env proposal. However as it stands, I believe we shouldn't deprecate this module.

@Leokuma
Copy link

Leokuma commented Sep 2, 2023

a .env file to be parsed as a configuration object

Wouldn't JSON or YAML be more appropriate for that? Because the purpose of an env file is to set the environment.

ability to log or output all env vars in use without knowing what the individual env var keys are

Is there a use case for that other than debugging?

This means there is no dependency on allowing env permissions

Could be achieved with --env too. And actually dotenv requires --allow-read, so we're basically swapping one permission flag for another.

useful for setups where the .env file is dynamically sourced for each environment

Fair point. How worse would it be to use a config.json or env.json instead?

Regarding env.example and env.defaults, I don't like their whole concept because they are magic names. It's not obvious that they are being used for anything at all, it's not explicit. I also don't like the cascade mechanism where one file overwrites configs from the other implicitly. I think it makes it harder to figure out the resulting env config.

Also I don't quite like the fact that I can call dotenv.load() at any point in my app and change the entire state of my app. I would prefer to have it crystal clear at the command line.

@jsejcksn
Copy link
Contributor

jsejcksn commented Sep 2, 2023

Toward a potential future: If it is ultimately decided that dotenv should be deprecated, I wonder if @pietvanzoen will un-archive the original source so that it can be updated from the work that's been done in std, and that maintenance can continue in userland — it's obvious that this is a useful collection of tools.

@cknight
Copy link
Contributor Author

cknight commented Sep 4, 2023

And actually dotenv requires --allow-read, so we're basically swapping one permission flag for another.

Good point!

Regarding env.example and env.defaults, I don't like their whole concept because they are magic names.

I would wholly agree here. This is something which has never sat well with me, as it goes against an ethos of Deno to avoid such things. Post your comments, I've been rethinking my stance on this module and am less supportive now of keeping it in std. @jsejcksn's proposal of moving this to userland certainly bears consideration (and would match the node environment where dotenv is a 3rd party module).

@pietvanzoen
Copy link
Contributor

Hey @jsejcksn. Happy to unarchive if that's the conclusion. Just let me know.

@cknight Funny you mention node because I just read that 20.6 will ship with native support for .env: https://twitter.com/kom_256/status/1692225622091706389

@cknight
Copy link
Contributor Author

cknight commented Sep 4, 2023

Yes, looks like both Deno and Node will support this from the CLI which is great. And in both environments, the dotenv library offers more functionality than the CLI will, so it makes some sense for that library to live on in some fashion.

@andrewthauer
Copy link

andrewthauer commented Sep 5, 2023

I'm not disagreeing with the points already made. JSON, etc. could work for some use cases, but may not be as ergonomic to work with imo.

For example, I have a script takes a .env file that contains placeholder values which are then resolved via a secret manager. This script/cli needs to resolve the values from the secret manager, parse the results, then run something akin to a envsubst on the values before loading the env. This is unlikely something a --env flag would handle. I can agree this might be an uncommon use case, but having the dotenv module in stdlib makes it a breeze with zero extra deps. Although, a userland dotenv library would be an option here.

Another consideration would be if you've written a CLI in Deno and compile it for easy distribution. Would this custom CLI be able to load a .env file even though there is no deno CLI in this case?

@iuioiua
Copy link
Collaborator

iuioiua commented Nov 22, 2023

Good points across the board here. I think std/dotenv is mainly used for the most basic use case of load(). That's just based on what I've seen across modules and projects. As stated, that functionality has been replaced in the CLI with the new --env CLI argument. There are what appears to be less common but valid use cases for other functionality provided in std/dotenv.

That all being said, I agree that we should deprecate std/dotenv in favour of a userland module that takes care of these more advanced use cases.

@andrewthauer
Copy link

Will it be possible to leverage the --env load with a compiled deno binary? I'm struggling to see how this would work. Probably the strongest case for having it part of std if not possible imo.

@iuioiua
Copy link
Collaborator

iuioiua commented Nov 23, 2023

Actually, Yoshiya and I will call about this and try to come to a resolution soon. I had some holes in my knowledge of the API and think there's a better way to do things.

Will it be possible to leverage the --env load with a compiled deno binary? I'm struggling to see how this would work. Probably the strongest case for having it part of std if not possible imo.

Yep, just using deno compile --env [script].

@timreichen
Copy link
Contributor

I still think we should remove all .dotenv related stuff (load, loadSync, etc.) but keep parse and stringify, which are useful.
Is the key value pair file format that dotenv uses a general spec? if so we could also rename the mod to that general spec.

@iuioiua
Copy link
Collaborator

iuioiua commented Nov 23, 2023

What are our current stances on this? Should we keep std/dotenv or remove it?

👍 for keep
👎 for remove

@kt3k
Copy link
Member

kt3k commented Nov 28, 2023

Looks like there's no consensus on deprecating it. Let's close this for now.

We just added --env feature very recently in Deno CLI. I think the community's stance could be changed in the future if --env has a lot of adoptions. Let's revisit this topic in that case

@kt3k kt3k closed this as not planned Won't fix, can't repro, duplicate, stale Nov 28, 2023
@iuioiua
Copy link
Collaborator

iuioiua commented Nov 29, 2023

For those interested in std/dotenv, PTAL at #3874. It's a proposal for a new prepare() aimed at superseding load().

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

No branches or pull requests

8 participants