Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

And add another section header as:

## Support for `.env` files

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's maybe make it a subsection as:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:

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

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

   	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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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