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

suggestion: deprecate std/dotenv's load() #4019

Open
iuioiua opened this issue Dec 22, 2023 · 17 comments
Open

suggestion: deprecate std/dotenv's load() #4019

iuioiua opened this issue Dec 22, 2023 · 17 comments
Labels

Comments

@iuioiua
Copy link
Collaborator

iuioiua commented Dec 22, 2023

Preface

#3604 was for discussing the deprecation of the whole std/dotenv sub-module. parse() and stringify() seem worth keeping. This issue specifically discusses the deprecation and removal of std/dotenv's load().

I'll aim to be concise. Those with feedback, please also aim to do the same so we can finally reach a desirable conclusion.

Problem

The solutions that load() provides are more complicated and opinionated than necessary. These solutions also overlap with current and possible future solutions, which are simpler and more robust. These solutions come at a cost (see references below).

load() mostly does the following:

  1. Loads a .env file into the current process. This overlaps with the runtime's new --env feature.
  2. Defines a set of environment variables required within the current process using a .env.example (or similar) file. Using a .env file to define this configuration is magic and seems unfitting for a Standard Library.
  3. Defines default values for environment variables in the current process using a .env.defaults (or similar) file. Again, using a .env file to define this configuration is magic and seems unfitting for a Standard Library.

Suggested solution

Offer simpler, smaller and more robust alternatives:

  1. Deprecate load() for removal in v1 of the Standard Library in favour of the --env flag. Independently, a third-party module could host this functionality.
  2. Implement a way to check whether a specified environment variable is set within the current process. feat: std/env with assertEnvSet() and setEnvDefault() #3874 was one possible way to do this.
  3. Encourage directly checking the current process if an environment variable is defined.

References

CC @timreichen, @jsejcksn, @cknight, @andrewthauer and @Leokuma

@iuioiua iuioiua added the dotenv label Dec 22, 2023
@Leokuma
Copy link

Leokuma commented Dec 22, 2023

I agree 100% with OP.

Users will still be able to use dotenv.load() from a third party, just like we do in Node as Node doesn't have a builtin .env functionality (aside from the recently added --env-file). Node's Dotenv is actually third party.

.env.example, besides being magical, is limited and misleading. We often have a cenario where a certain variable becomes required or optional depending on the value of other variables, and you can't express that in a .env.example. Also, the fact that a required variable is present in .env doesn't mean its value is valid, so code validation is still necessary anyway. The same logic can be applied to .env.default: one variable's default value might depend on another variable. Also, cascading ini/env files makes it difficult to figure out the resulting env because then you have to find multiple files and basically do a diff.

Personally I only need parse(), stringify() and maybe an async open(envfile: string): Record<string, string> function that takes the filename (required) and reads and parses the specified env file, but doesn't touch Deno.env. That way I can even give precedence to the OS's env variables over .env (not sure if that would be a good idea, though).

To me, autodiscovering files - even a .env - feels like an antipattern in Deno.

Thanks for tagging me, and sorry for not replying earlier. I did read all the comments in the PR carefully.

@kt3k
Copy link
Member

kt3k commented Dec 23, 2023

To me, autodiscovering files - even a .env - feels like an antipattern in Deno.

Can you elaborate? Autodiscovery of .env is, in my view, the main point of std/dotenv, and that is a major difference of std/dotenv and CLI's --env.

@Leokuma
Copy link

Leokuma commented Dec 23, 2023

I think autodiscovery in general is not aligned with Deno's philosophy of being explicit. Deno doesn't autoload import maps for example.

There's no explicit connection between the .env file and the codebase, meaning that a CtrlF will not find anything related to the .env file. The only way the dev can know that the .env file is being used at all is knowing it beforehand. I know that .env autoloading is a well known pattern, but we don't have to perpetuate legacy ways of doing things. Young devs may not know about that pattern and will be puzzled.

@kt3k
Copy link
Member

kt3k commented Dec 24, 2023

Deno doesn't autoload import maps for example.

Deno autoloads deno.json since v1.18 and the import map embedded in it is auto-applied.

I generally agree that Deno prefers explicit over implicit, but it also allows auto-discovery of things when practicality wins.

I know that .env autoloading is a well known pattern, but we don't have to perpetuate legacy ways of doing things.

I think I find this pattern more often in modern tools. Some examples are prisma remix docker compose, fresh, etc. I feel auto-discovery of .env is growing trend, rather than a legacy bad practice.

@Leokuma
Copy link

Leokuma commented Dec 24, 2023

Regarding the trend of .env autoloading, there's also a heavyweight precedent against it: NodeJS.

In terms of practicality, the difference would be that the user would have to add --env:

deno run --env=.env main.ts

Or possibly use deno.json when available:

{
  "env": ".env"
}

I believe the extra effort is worth it. That way the only magical untraceable step is the deno.json autoloading. Everything loaded from deno.json is not magic because it's explicitly stated in it.

Merry Christmas! 🎄

@iuioiua
Copy link
Collaborator Author

iuioiua commented Dec 26, 2023

We have --env, which comes for free with the runtime and makes the end-user of any code explicitly aware of any .env file being used. This is a good thing.

load() requires --allow-read, --allow-env and an added dependency. It's also possible for load() to be used in code and have the end user unaware of a .env file being used. For this reason, autoloading is arguably inferior to --env from a security standpoint. Even if autoloading were seen as a feature, its benefits are minuscule and don't seem substantial enough to have it in the Standard Library.

@kt3k
Copy link
Member

kt3k commented Dec 28, 2023

@Leokuma

Or possibly use deno.json when available:

{
  "env": ".env"
}

Sounds interesting idea to me, worth pursuing in Deno CLI repo.

@iuioiua

Even if autoloading were seen as a feature, its benefits are minuscule and don't seem substantial enough to have it in the Standard Library.

dotenv load is used in many place across ecosystem. (ref. https://github.com/search?q=std%2Fdotenv%2Fload&type=code ) Deno core team also internally uses it in many places including fresh init template. I don't understand why we can judge it in that way.

@kt3k
Copy link
Member

kt3k commented Dec 28, 2023

Also I haven't seen people having migrated from std/dotenv to --env yet. I think this suggestion only makes sense when the majority of existing users of std/dotenv/load migrated to --env and started considering std/dotenv redundant. Otherwise the deprecation of it would cause lot of confusion as --env doesn't provide equivalent functionality of std/dotenv/load.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Dec 28, 2023

Sounds interesting idea to me, worth pursuing in Deno CLI repo.

+1

dotenv load is used in many place across ecosystem. (ref. https://github.com/search?q=std%2Fdotenv%2Fload&type=code ) Deno core team also internally uses it in many places including fresh init template. I don't understand why we can judge it in that way.

Fresh can use --env within the start task. I've submitted denoland/fresh#2204 to do that.

Also I haven't seen people having migrated from std/dotenv to --env yet. I think this suggestion only makes sense when the majority of existing users of std/dotenv/load migrated to --env and started considering std/dotenv redundant.

Deprecating load() for removal in v1 of the Standard Library will facilitate the adoption of --env and therefore spur improvements to that functionality thanks to greater usage. Removal in v1 would be the most appropriate time to make the move. I'd also suggest highlighting this deprecation in the next runtime version release if we decide to go ahead with it.

Otherwise the deprecation of it would cause lot of confusion as --env doesn't provide equivalent functionality of std/dotenv/load.

I believe my previous points and suggestions address the concerns regarding the differences in functionality.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Mar 27, 2024

Furthermore, removing load(), which uses Deno.readTextFile[Sync](), cannot be used in other runtimes. If load() were removed, we could add other runtimes to the list of runtimes supported in std/dotenv.

@kt3k
Copy link
Member

kt3k commented Mar 28, 2024

I personally think if we deprecate load, we should deprecate the entire dotenv. I've never seen the usage of dotenv other than load/load.ts

@jsejcksn
Copy link
Contributor

I personally think if we deprecate load, we should deprecate the entire dotenv. I've never seen the usage of dotenv other than load/load.ts

I've rarely used load, but often used the other tools for the reasons I described here: #3874 (comment)

@andrewthauer
Copy link
Contributor

I use parse / stringify in a few places. Have never used load.

@kt3k
Copy link
Member

kt3k commented Apr 8, 2024

@jsejcksn #3874 (comment)

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.

Can you give the example usage of this pattern?

@andrewthauer Can you elaborate on your usage pattern? Do you read text from .env file?

@andrewthauer
Copy link
Contributor

andrewthauer commented Apr 8, 2024

@kt3k - I have 2 cases where parse specifically is useful.

  1. Transformations and processing on the .env file format. Its becoming more common the store secret reference values vs actual secret values. Being able to parse, iterate and resolve these value references is key to doing this. It was suggested another format like json could be used for this, but the .env file format is well known and convenient to edit for this purpose.

  2. Using in your own CLI. Once you want to parse custom args you can't nicely pass those to deno --env. Further more, when compiling a Deno based CLI the arguments passed to compile command are baked into the compiled binary execution and there is no way to do --env /path/to/.env then pass to deno run. You need to manually read the file and load it into the runtime env. Having parse makes this easier.

As for stringify it's less useful but I have used it before and seems like you would want a n analogue to parse.

@jsejcksn
Copy link
Contributor

jsejcksn commented Apr 8, 2024

@jsejcksn #3874 (comment)

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.

Can you give the example usage of this pattern?

@kt3k Calling the load function is the most common example I can imagine.

@iuioiua
Copy link
Collaborator Author

iuioiua commented Apr 12, 2024

PTAL at #4578, where I suggest we simplify load()'s behavior in std/dotenv@1 instead of deprecating it.

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

No branches or pull requests

5 participants