I haven't tested but I assume that dotenv.config() is safe to call when env file at path doesn't exist.

If it's the case I would just run it unconditionally

Technically we need a file exists check only to eventually display deprecation message so with v3 it'll be nice to remove this checks.

I take it can be constructed as follows

if (useDotEnv) {
  dotenv.config(...)
} else {
  cost hasDotEnvFile = ...
  if (hasDotEnvFile) showDeprecation()
}

I like that approach, however, that suggests that you'd like the behavior to be a bit different than the one in serverless/components - please see my question from the above

By following the similar approach as serverless/components, only one .env file can be loaded - would it make sense to load both .env and .env.stage if there're present in a way that .env.stage will be loaded later to override overlapping variables from .env?

So, do you think that we should load both env files (default one first, then stage) if they exist?

@pgrzesik what about that one

If I read correctly from dotenv implementation, they return object with error property if file was not found: https://github.com/motdotla/dotenv/blob/7301ac9be0b2c766f865bbe24280bf82586d25aa/lib/main.js#L108

This also reminds me, that it'll be good to signal any parsing errors to the users. I think it'll be good to crash then with meaningful error.

If I read correctly from dotenv implementation, they return object with error property if file was not found: motdotla/dotenv@7301ac9/lib/main.js#L108

Do you suggest using that to avoid explicit checks if the file exists? If yes, then, unfortunately, we still have to check for the existence of files in case the useDotenv is not set to true.

This also reminds me, that it'll be good to signal any parsing errors to the users. I think it'll be good to crash then with meaningful error.

That's a good idea, however, I couldn't really come up with any actual possible parsing errors from dotenv.config that are not related to nonexistent file under provided path. I still think it's a good idea to check for any error. How would you like the framework to handle it? Crash or rather emit a warning to the user and continue processing as "normal"?

If yes, then, unfortunately, we still have to check for the existence of files in case the useDotenv is not set to true

Yes, but it's temporary, it's only for deprecation reporting and will be removed with next major

How would you like the framework to handle it? Crash or rather emit a warning to the user and continue processing as "normal"?

I think we should crash. If env setup is broken it's unlikely things will go well

Let's maybe name it as "Resolution of environment variables"

And add another section header as:

## Support for `.env` files

Changed 👍

It'll be good to inform upfront that it's an opt-in. So maybe let's put it as:

With useDotenv: true set in your serverless.yml file, framework automatically... (from next major version .env files will be loaded by default and useDotenv setting will be ignored)

Changed 👍

Let's maybe change current directory to service directory, it'll be a bit more specific

Changed 👍

Let's maybe make it a subsection as:

###  Differences against  `serverless-dotenv-plugin`

Changed 👍

I'm not sure if creating a class is justified in this case. It looks as relatively simple loadEnv function encapsulated into class, which doesn't feel right.

Let's host it as function, same as e.g. eventuallyUpdate.js logic

Still I notice that utils folder also doesn't seem right, to me it can also be placed directly in lib (we anyway need at some point to improve internal organization)

I was also not sure but I decided to follow similar approach as with YamlParser class. I currently changed implementation from class-based to function-based, however, it still depends on serverless instance. I was thinking on how to separate it, but it actually requires a few dependencies such as access to serverlessConfigFile, options and logDeprecation. I'm on the fence here and I see a few options:

• keep the dependency on serverless
• keep the dependency on specific pieces - instead of passing serverless instance, just pass the needed pieces
• resolve everything in Serverless and only pass stage, useDotenv to loadEnv function

Which one do you think makes the most sense here? I personally like the option #3, but that moves some of the responsibilities of loadEnv directly to Serverless class, which is not ideal

I was also not sure but I decided to follow similar approach as with YamlParser class

Totally understood, we have a lot of legacy conventions. Still it's good to abort some of them, especially when they resemble noisy not well justified practices.

it still depends on serverless instance

That's true. I think the cleanest approach for now, would be to follow with something as:

InServerless.js (inline in init or in new method):

if (serviceContext && serviceConfig.useDotEnv) {
  const stage = // resolve stage
  loadDotEnv(stage);
} else {
  isDotFileEnvExisting = // fs exists checks
  if (isDotFileEnvExisting) showDeprecation();
}

And in lib/resolveDotEnv.js just load eventually existing config for given stage

Note, that if we do it that way, then we would be able to use lib/resolveDotEnv.js in as is form, in refactor we plan to do here: #8364 (it is mostly about separating CLI and programmatic logic, so Framework core is purely programmatic (there will be then no dotenv resolution in its core), it'll also allow to integrate Components better)

So yes, option 3 as you pointed, seems best :)

But without passing useDotenv (let's call the util when we know we want to resolve env from .env)

Thanks for a comprehensive explanation, I wasn't sure if moving some of the env-related logic makes sense in this case, but thanks to your explanation I see the reasoning and I also think that this is the most appropriate solution moving forward 🙇

@pgrzesik what about that one

If I read correctly from dotenv implementation, they return object with error property if file was not found: https://github.com/motdotla/dotenv/blob/7301ac9be0b2c766f865bbe24280bf82586d25aa/lib/main.js#L108

This also reminds me, that it'll be good to signal any parsing errors to the users. I think it'll be good to crash then with meaningful error.

Having it as util (which just takes servicePath as an input) we may write simpler test not involving runServerless.

Then we can write temporary files to home dir (it's a temporary folder in test run, not real user home dir), and handle process.env with process-uils/override-env as:

overrideEnv(() => {
  // Load env from .env
  // Confirm on `process.env` it's effective
});

It'll be also helpful to have it detached from Serverless instance, as it's likely we will refactor whole process follow up to this proposal: #8364

Depending on the decision on dependencies of loadEnv func, I will either refactor it to not depend on serverless or keep it in current form

This is a place that I would love to get your feedback @medikoo - I was trying to just setup .env files once in before, however, after first test, the files were no longer accessible, so I switched to beforeEach/afterEach combo that I'm not super happy about due to unnecessary creation of .env files before each test. Do you have any suggestions on how to properly set it up so these files setup in before will be visible to all tests?

@pgrzesik As we're preparing it as standalone module (that takes stage), the unit test may simply put needed .env files to process.cwd(), and confirm that after running util expected env vars are seen on process.env

Important notes:

• process.env is reset of each test file, therefore we don't have to worry about side effects (it's ensured by: restore-env Mocha extension we have loaded
• process.cwd() is set to home dir, which is set to temporary dir (again ensured by Mocha extensions: mock-cwd and mock-homedir - note that homedir is cleanup up for other test files, so we also do not need to take care of cleaning it.

Additionally if you'd like to have process.env or process.cwd isolated for specific test runs (so there are no side effects for following tests in same test file). check: process-utils/override-env an process-utils/override-cwd

When having it isolated, and tested as above with unit test, not sure if it's worth testing that resolution works in context of Serverless instance. We might have some one sanity test, and such can be constructed as:

1. Setup a chosen fixture (like here:

   serverless/lib/plugins/aws/package/compile/functions/index.test.js

   Line 2931 in c4c57b0

   const { servicePath, updateConfig } = await fixtures.setup('function');

   )
2. Then in returned servicePath setup needed .env files
3. Invoke runServerless run on fixture (as here:

   serverless/lib/plugins/aws/package/compile/functions/index.test.js

   Lines 2933 to 2936 in c4c57b0

   	const { cfTemplate: originalTemplate } = await runServerless({
   	cwd: servicePath,
   	cliArgs: ['package'],
   	});

   )
4. Confirm on the outcome

If that doesn't answer your questions, let me know. I know I've provided an indirect answer, but I hope that above information clarifies how to setup test efficiently

I've posted above comment, without looking into test file, which looks very good imo.

I believe you don't need to create tmp dir, you may just put files in process.cwd() (once) as is, and all that you need to reset is process.env (which you already do with process-utils/override-env).

.env files should definitely not be removed between single tests, so you should be able to setup them once in before (they will however be cleaned after whole test file runs)

Here, as suggested, I changed the approach to try to load files and based on error returned by dotenv.config decide how to proceed further. I'd love to hear your opinion on the chosen approach and I'm open to suggestions on how to improve it further

It looks great!

I've posted above comment, without looking into test file, which looks very good imo.

I believe you don't need to create tmp dir, you may just put files in process.cwd() (once) as is, and all that you need to reset is process.env (which you already do with process-utils/override-env).

.env files should definitely not be removed between single tests, so you should be able to setup them once in before (they will however be cleaned after whole test file runs)

Let's use async function (and rely on fs.promises directly).

(Unfortunately in Framework we have some not great old patterns that dates Node.js v4 and before, we will slowly recover from that, and ideally if new code is free from them)

Same here let's rely on fs.promises.writeFile

More logical check is to.not.have.property() (theoretically undefined can be assigned as value, and that should be not expected)

I'd probably start each sentence capitalized.

also let's fix functions' into function's

I'd probably reword it to:

Failed to load environment variables from "${filePath}": ${error}"

• Original error message can be verbose, so it's better to put this at the end
• It's also convention in few other places, to first provide short context info, and then trail it with original error message.