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

Managing large amounts of environment variables #28661

Open
Lindsor opened this issue Oct 17, 2024 · 13 comments
Open

Managing large amounts of environment variables #28661

Lindsor opened this issue Oct 17, 2024 · 13 comments
Labels
area: @angular/build feature: votes required Feature request which is currently still in the voting phase feature Issue that requests a new feature

Comments

@Lindsor
Copy link

Lindsor commented Oct 17, 2024

Command

build, serve, test, e2e

Description

I am reopening #4318.

Essentially we need a way to pass environment variables to the build/serve commands. environments.ts file is not a viable solution as you would need to end up with hundreds of environment.ts files depending on all the permutations per environment.

The solution to #4318 also does not address this since you need to know the define values when running the command ng build <define> <define> <define>....
Which means you would have to generate the build command same way as generating the environment.ts file.

Most enterprise angular applications I have worked on ends up extending the webpack.config.js file adding:

DefinePlugin({
  process: {
    env: {
      VAR_1: JSON.stringify(process.env.VAR_1),
      ...
    }
  }
})

This issue has been brought up for years with the Angular repo but i've never seen a resolution so I want to try to make it as clear as possible.
We should support a dotenv() style solution where in the code we can use process.env.VARIABLE.
In the build machine we can either:
export VARIABLE=test
or have a .env file to pick it up.

This is supported by pretty much every other framework out there so I'm not sure why it wouldn't be possible or hasn't already been implemented.

If the issue is the angular team doesn't want this behaviour then that is fine but I am looking for a yes/no type answer. If the answer is no the angular team would be able to point to this issue for all future feature requests of this nature.

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Oct 17, 2024

With --define you can use environment variables, doesn't the below cover your use-case?

export VAR_1=“foo”

$ ng build --define="VAR_1='$VAR_1'"

We recently discussed using import.meta to provide environment variables, and he had some security concerns about limiting access to certain variables. Directly in code access to process.env in a web context should be avoided. Despite its increasing popularity, I believe this is both a bad practice and an anti-pattern. process.env is specific to Node.js and as such should not be exposed or used in other ecosystems, as it can introduce security risks and lead to unintended behavior in web applications.

@clydin
Copy link
Member

clydin commented Oct 17, 2024

The original request for issue #4318 was asking for a way to pass system environment variables from the command line as shown by this example from the original issue: ng build --prod --envVar:commitId: CI_COMMIT_ID. This is now possible via --define. The --define option can be used to bring in selective environment variables as well as things like a build timestamp or the git hash for the build. The full capabilities of the shell are available. This is quite common with other compilers that have some form of a define option (e.g., clang/gcc/etc.).

For cases where there is a larger list of values, a JSON file that is imported into the application at needed locations is an option and a potential replacement for the old environment file concept. Not only can the JSON file be tree-shaken based on usage but there is also full IDE support and build type-checking to ensure values are used as expected within the application. Combined with the imports field in a project's package.json (or a nested package.json), conditions can be used to adjust the resolved file at build time. Currently for v19, development/production conditions are automatically available based on the optimization settings of the build. For SSR use cases, the browser condition is also available. Further expansion of this is also being considered with concepts such as per build configuration customization of the active conditions.

As an example, here is a package.json excerpt and usage in an application file:

{
  "imports": {
    "#config": {
      "development": "./app/dev-config.json",
      "default": "./app/prod-config.json"
    }
  }
}

import { Component } from '@angular/core';
import { value } from '#config';

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrl: './app.component.css',
})
export class AppComponent {
  title = 'v19' + value;
}

@dgp1130
Copy link
Collaborator

dgp1130 commented Oct 17, 2024

Just to expand on some of the implicit requirements of this request, it seems like --define isn't sufficient for use cases which:

  1. Have to set a lot of variables.
  2. Those variables change between environments.
  3. Those variables should not be checked in to source.

Command line --define doesn't scale great with large numbers of variables (fails 1.) while hard-coding define in angular.json needs to be checked-in (fails 3.).

I agree dotenv is a potential solution to this problem. I also like @clydin's suggestion with import conditions. You can similarly define one file per environment and .gitignore them just like dotenv files.

One potential benefit to dotenv is configuring environment variables on a production server at runtime by giving it a new dotenv file after the Angular build. I'm not sure we'd want to support that since it seems like it could be confusing to build with one set of variables and then overwrite them with something else at runtime. Also that would diverge the dotenv variables available on the client and the server, which could be tricky to work with. I think the better approach would be to work with your deployment provider to set environment variables based on some configuration (though they may want dotenv for that configuration anyways).

@dgp1130 dgp1130 changed the title True Environment Variable Support Managing large amounts of environment variables Oct 17, 2024
@dgp1130 dgp1130 added feature Issue that requests a new feature area: @angular/build labels Oct 17, 2024
Copy link
Contributor

angular-robot bot commented Oct 18, 2024

This feature request is now candidate for our backlog! In the next phase, the community has 60 days to upvote. If the request receives more than 20 upvotes, we'll move it to our consideration list.

You can find more details about the feature request process in our documentation.

@angular-robot angular-robot bot added the feature: votes required Feature request which is currently still in the voting phase label Oct 18, 2024
@Lindsor
Copy link
Author

Lindsor commented Oct 18, 2024

With --define you can use environment variables, doesn't the below cover your use-case?

export VAR_1=“foo”

$ ng build --define="VAR_1='$VAR_1'"
We recently discussed using import.meta to provide environment variables, and he had some security concerns about limiting access to certain variables. Directly in code access to process.env in a web context should be avoided. Despite its increasing popularity, I believe this is both a bad practice and an anti-pattern. process.env is specific to Node.js and as such should not be exposed or used in other ecosystems, as it can introduce security risks and lead to unintended behavior in web applications.

Honestly @dgp1130 explained it much better then I did. Essentially --define does not scale well because it has to be embedded into the build command. Which means for variable number of define's we would need a script that generates the full build command, this is essentially the same thing thats possible now by generating the environment.ts files. Its not very scalable.

If security is a concern (accidentally leaking environment variables that shouldnt be) then we can copy react/nextjs where only variables starting with NG_ will be replaced in the code. For the environment.ts file generation I spoke about above I've always seen people do it based on environment variables anyway so to me this does not introduce more security risk. If we adopt the NG_ naming convention if anything it reduces the security implication because now developers will think about the variables they are exposing more.

If you look at the original ticket the conversation continued even after the PR was merged indicating it did not address the issue for a lot of users.

I disagree with @clydin suggestion though since to me that is the same thing as the environment.ts file. They are static files that will need to be generated since if your working on 10 different environments which each requires different settings it'll be unmaintainable to have every permutation in there.

@clydin
Copy link
Member

clydin commented Oct 18, 2024

Can you expand on your actual use case? How many environment variables are typically used? Won't the values need to be generated somewhere in all cases as well?
Having a thorough understanding of the problem space will help to ensure a comprehensive solution.

@Lindsor
Copy link
Author

Lindsor commented Oct 18, 2024

Can you expand on your actual use case? Don't the values need to be generated somewhere in all cases?

So the most recent use case is for an Ionic/Angular Hybrid Application (1 codebase = web app + android app + ios app).
We have ~10 environments each running different enterprise projects which means any environment can have:

  • Same or different app code version
  • Same or different build-time environment requirements

Lets use the example of the same app version being deployed on 3 environments.
Environment 1:
API needs to point to: https://example1.com
API Vendor needs to point to: https://vendor.stage1.com
Vendor JS: https://vendor1.js

Environment 2:
API needs to point to: https://example2.com
API Vendor needs to point to: https://vendor.stage1.com
Vendor JS: https://vendor2.js

Environment 3:
API needs to point to: https://example3.com
API Vendor needs to point to: https://vendor.stage2.com
Vendor JS: https://vendor3.js

Notice the API points to a different domain on each environment.
The vendor API can be the same or different per environment
The Vendor JS can be the same or different per environment

The vendor JS needs to be included in the index.html we CANNOT do a injectScript(vendor.js) which is why runtime config doesnt work here. (Vendor requirement not something we can choose)

Since this is run on a mobile app even if we wanted to load a runtime config we would need to have hardcoded domain of where to load the configuration since domain based path routing wouldn't work, but this issue would exist on Angular universal running on the server as well my example is just mobile but there would be other use cases.
And keep in mind above I only included 3 "variables" which is not a lot but think more like 10+.

The environment variables would be set on the build machine or CI/CD pipeline configuration. So our devops team can configure:
Build+Deploy Job
API=https://example1.com
API_VENDOR=https://vendor.stage1.com
VENDOR_JS=https://vendor3.js

They can run the Build+Deploy Job 3 times and just change the required environment variables for the app to be built correctly.

@clydin
Copy link
Member

clydin commented Oct 18, 2024

Thank you for the additional information. We will be discussing this further with the team and attempt to provide an ergonomic solution to these type of use cases.

As to prefixing, NG_ prefixed environment variables are already used to control advanced/debug features of the CLI and build system so repeat use there would be problematic. This is actually an example of the underlying concern with using environment variables this way. They are effectively global variables at the Operating System level that can effect any active code on the system. NODE_ENV, for instance, is near universally considered a mistake at this point.

If dotenv files are a preferred way of storing key/value pairs then there is the potential for an additional define-based option (e.g., --define-file=, or just --define with a file name). This could be used to pass in a specific .env file that would be parsed and populate the actual define option. Questions on precedence would need to be considered. Though as with any define based solution, IDE support and type-checking is limited which is overall more error-prone from a build correctness perspective. This is a hypothetical addition and may not be actualized but it appears viable.

@Lindsor
Copy link
Author

Lindsor commented Oct 18, 2024

Thank you for the additional information. We will be discussing this further with the team and attempt to provide an ergonomic solution to these type of use cases.

As to prefixing, NG_ prefixed environment variables are already used to control advanced/debug features of the CLI and build system so repeat use there would be problematic. This is actually an example of the underlying concern with using environment variables this way. They are effectively global variables at the Operating System level that can effect any active code on the system. NODE_ENV, for instance, is near universally considered a mistake at this point.

If dotenv files are a preferred way of storing key/value pairs then there is the potential for an additional define-based option (e.g., --define-file=, or just --define with a file name). This could be used to pass in a specific .env file that would be parsed and populate the actual define option. Questions on precedence would need to be considered. Though as with any define based solution, IDE support and type-checking is limited which is overall more error-prone from a build correctness perspective. This is a hypothetical addition and may not be actualized but it appears viable.

Haha i agree NODE_ENV was definately a mistake.
--define-file=./file is definately a step in the right direction but still not quite there.
I would still need to set all my variables in the CI/CD GUI then have a generate script to put those into 1 file, at which point its just another generation same as we have today.

NG_ being already used makes sense so something like NGX_PUBLIC or w/e we come up with. I get the concerns of these being global but thats what environment variables entire job is, looking at almost every big name framework out there they support it pretty simply so if we think through the naming convention correctly i dont think it will cause that many issues

@clydin
Copy link
Member

clydin commented Oct 18, 2024

The team will still be discussing this issue. But from further reading of the above details, a short script that runs pre-build may be a strong option for that use case. Something similar to the following could be used:

const PREFIX = "MY_CUSTOM_CONFIG_PREFIX_";
const CONFIG_PATH = 'config.json';
const config = Object.create(null);
Object.entries(process.env).forEach((entry) => {
  if (entry[0].startsWith(PREFIX)) {
    config[entry[0].slice(PREFIX.length)] = entry[1];
  }
});
require("fs").writeFileSync(CONFIG_PATH, JSON.stringify(config), 'utf-8');

And this could be added to the build npm script in package.json:

    "build": "node ./generate-config.js && ng build",

The advantages here would be complete flexibility as to the prefix (or even an allow list instead), the ability to adjust values, add entries from multiple sources, add defaults, or fail early if required entries are missing. A development only JSON file could even be checked in with local development values that would be overwritten on CI. Since it would be an imported JSON file, IDE support and build time diagnostics would be possible and full details of all usages throughout the application would be available. Plus this setup allows for a transition to runtime configuration in the future, if preferred, by moving the generated file to an asset that is served instead of imported.

@Lindsor
Copy link
Author

Lindsor commented Oct 18, 2024

@clydin yep this makes sense and its a very elegant solution. Its essentially what we are currently doing just less organized i would say.
Even something like that if it was embedded as part of ng build command would solve this issue i would say. Since in package.json we could have:

{
  "scripts": {
    "build": "dotenv() ng build"
  }

That way angular itself is not dependent on dotenv (which i think is smart) but applications can still leverage it very easily if they decide to use it.
And you would not need to care about priority of environment variables or any of that complexity, it would be offloaded to the consuming app's choice.

@clydin
Copy link
Member

clydin commented Oct 18, 2024

From your use case from above, I thought dotenv would not be a viable solution since it would necessitate generating .env files for each permutation of the build?

The downside of adding something like what i described into the build system itself is that it removes all of the flexibility and extensibility that I mentioned in the last paragraph.

@Lindsor
Copy link
Author

Lindsor commented Oct 18, 2024

Good point I should clarify that if a project WANTS to use dotenv it would work. For my use case you are correct we would not have .env files in the pipelines but having them in local would work.
If dotenv doesnt detect a .env file then it just loads the exported variables it doesnt fail (i think its been a while since ive looked at the code) so that way it would work on both pipelines and local development.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: @angular/build feature: votes required Feature request which is currently still in the voting phase feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests

4 participants