fix(dotenv/types): allow null for *path values

[jsejcksn]

This PR adds null to the union of allowed types for the path-related options in the LoadOptions type.

This change fulfills two objectives:

1. The current type uses optional properties for these string values. This means that the values can be provided as strings — or not provided at all and will become undefined at runtime.

   Note that it's also possible to explicitly provide undefined as a value as long as the compiler option noUncheckedIndexedAccess is not enabled — It is a stronger type safety option, but is unfortunately not enabled by default in Deno.

   Because the functions which use these properties also implement default parameters in the case that they are not defined (e.g. load and loadSync), the values will always be strings at the usage sites. In order to prevent usage of each of the default parameter values, one must currently pass the empty string "". This works, but is a code smell — it does not clearly indicate intent — so including null in the union of allowed types will allow the programmer to explicitly indicate "I don't want to use a path here — not even a default value".

2. A nice side-effect of the use of null to opt out of specifying a path is that the program no longer needs read permissions for the default path parameters in that case. If a programmer would like to use this module to load environment variables from only a single file, it allows for stricter permission control by only specifying that one file's path for the read permissions.

[jsejcksn]

Note to self and for a potential follow up PR: After refactoring the new test to be stricter, I realized that there doesn't seem to be a way to load environment variables purely from a file: without granting access to at least some actual environment variables. In my view, this seems like an oversight and could be fixed rather easily by:

• not using an empty array [] as the default parameter for restrictEnvAccessTo in most of the functions in this module, and
• refactoring readEnv to interpret the case of an empty array as "allow access to no variables" (a no-op), and just return {} without attempting to use the Deno.env API at all

[jsejcksn]

This is not strictly necessary, but I had to examine the entire test file to be sure that nothing was mutating these properties before I felt confident that I could rely on their values in an additional test. This change will prevent someone else from having to do the same.

[kt3k]

@jsejcksn
Sorry for the delay in review. This makes sense to me. Especially null values for examplePath and defaultsPath make sense for disabling them.

I wonder if supporting null for envPath is a good idea. I guess dotenv doesn't have a point if the user skip loading value from env file. What do you think?

[jsejcksn]

I wonder if supporting null for envPath is a good idea. I guess dotenv doesn't have a point if the user skip loading value from env file. What do you think?

Hi @kt3k: I think it is useful to support it. It might not seem obvious for typical cases — but why should we arbitrarily limit less common workflows when it's simple to provide granular, programmatic control?

For example: a user might generate the configuration from parsed CLI arguments or other code to support branched logic where, in some conditions, the program should not try to read the .env file at the default path (which might cause a permission error) and instead fallback to another (e.g. the one at defaultsPath).

[timreichen]

I agree with @kt3k. I think allowing null as envPath is not a good idea as the whole point of load() is to load an .env file. Passing null seems to be an anti pattern.

For example: a user might generate the configuration from parsed CLI arguments or other code to support branched logic where, in some conditions, the program should not try to read the .env file at the default path (which might cause a permission error) and instead fallback to another (e.g. the one at defaultsPath).

For that particular case: Couldn't envPath just be set as the defaultsPath instead of null to get the same result?

[jsejcksn]

@timreichen Thanks for the feedback.

I agree with @kt3k. I think allowing null as envPath is not a good idea as the whole point of load() is to load an .env file. Passing null seems to be an anti pattern.

For example: a user might generate the configuration from parsed CLI arguments or other code to support branched logic where, in some conditions, the program should not try to read the .env file at the default path (which might cause a permission error) and instead fallback to another (e.g. the one at defaultsPath).

For that particular case: Couldn't envPath just be set as the defaultsPath instead of null to get the same result?

Yes, for that particular example the technique you suggested would be a workaround, but it would require more complicated logic compared to simply toggling options on/off.

I agree that the purpose of load/loadSync is to load data from .env files — but they can be loaded from more than one source. Why is it an anti-pattern to support workflow strategies that differ from the most common case?

[kt3k]

@jsejcksn Thanks for clarifying the reasoning. Now this looks good to me.