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

#213 integrate dotenv for environment variable management #215

Merged
merged 21 commits into from
Nov 1, 2024

Conversation

trungnotchung
Copy link
Contributor

Integrates dotenv to load environment variables directly into the configuration, enhancing flexibility and reducing the need for manual parameter passing across different environments.

@elielnfinic
Copy link
Contributor

I don’t think using dotenv is a good approach for loading environment variables in this context, as it is a package that might be included by other JavaScript/TypeScript projects. Once the package is added to a project, it will be located in node_modules, and editing the .env file directly may not be ideal.

@d-roak
Copy link
Member

d-roak commented Oct 29, 2024

I don’t think using dotenv is a good approach for loading environment variables in this context, as it is a package that might be included by other JavaScript/TypeScript projects. Once the package is added to a project, it will be located in node_modules, and editing the .env file directly may not be ideal.

We had plans to add it. The reasoning you are adding does not make sense, the package is not just to be imported but also to run as a standalone, you want to support the standard ways to configure it

packages/node/src/config.ts Outdated Show resolved Hide resolved
packages/node/src/config.ts Outdated Show resolved Hide resolved
packages/node/src/config.ts Outdated Show resolved Hide resolved
packages/node/src/config.ts Outdated Show resolved Hide resolved
packages/node/.env.example Outdated Show resolved Hide resolved
Copy link
Contributor

@elielnfinic elielnfinic left a comment

Choose a reason for hiding this comment

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

As previously mentionedd, this PR should not be merged because we can't have a .env file in a library that can be included by other projects either in browser or as a node modules.

package.json Outdated Show resolved Hide resolved
@hoangquocvietuet
Copy link
Contributor

As previously mentionedd, this PR should not be merged because we can't have a .env file in a library that can be included by other projects either in browser or as a node modules.

Dotenv automatically configures settings when we run it locally. However, we can also pass configurations directly into the constructor when creating new topology nodes in other projects. This means topology nodes can operate with either default configurations or custom settings.

Copy link
Contributor

@elielnfinic elielnfinic left a comment

Choose a reason for hiding this comment

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

In standalone environments, looks good to me.

Copy link
Contributor

@JanLewDev JanLewDev left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@winprn winprn left a comment

Choose a reason for hiding this comment

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

LGTM

packages/node/.env.example Outdated Show resolved Hide resolved
packages/node/src/config.ts Outdated Show resolved Hide resolved
Copy link
Member

@d-roak d-roak left a comment

Choose a reason for hiding this comment

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

use undefined instead of empty object

packages/node/src/config.ts Outdated Show resolved Hide resolved
packages/node/src/run.ts Outdated Show resolved Hide resolved
Copy link
Member

@d-roak d-roak left a comment

Choose a reason for hiding this comment

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

lgtm

@d-roak d-roak merged commit fe9d267 into main Nov 1, 2024
6 checks passed
@d-roak d-roak deleted the feature/dotenv-config branch November 4, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants