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

v5 proposal #40

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

v5 proposal #40

wants to merge 1 commit into from

Conversation

lesnitsky
Copy link

Hello @mockturtl 👋

This is a proposal PR which is by no means final, but a prototype for better understanding of the underlying problems and proposed solutions:

Motivation

dotenv is used in many backend dart projects, and we've had a few support tickets raised at globe.dev regarding env vars loading.

includeParentEnvironment

I'm not sure why merging .env and Platform.environment is not a default behavior. Typically .env is used to override production env vars locally for development purposes. .env files are rarely checked in into version control system and cloud providers/hosting environments have their own secure way to define env vars. Bundling .env is less secure, since it's just a plain text inside a file inside an archive with the code.

This PR implements merging Platform.environment with whatever is defined in .env as a default behaviour.

Initialization API

Compared to NodeJS's require('dotenv').config(), current initialization API is a bit more complicated, requires calling both constructor of DotEnv and load(), which has 2 parementers.

This PR implements singleton pattern on DotEnv class, similar to how it's done in firebase/flutterfire. Reading env var becomes as easy as:

final myVar = DotEnv.instance['MY_VAR'];

Although I'm not sure when this could be useful, there's still a way to have a custom parser:

final dotenv = DotEnv(MyDotEnvParser())..load();
final myVar = dotenv['MY_VAR'];

Let me know what you think, if you're willing to accept these breaking changes, I'm happy to proceed with this PR, and update tests, docs, and readmes.

/// Prefer using [operator[]] to read values.
@visibleForTesting
Map<String, String> get map => _map;
final DefaultParser _parser;
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be of type Parser


import 'package:meta/meta.dart';
abstract class Parser {
Copy link

Choose a reason for hiding this comment

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

I guess this serve as an interface. So it could be abstract interface class Parser

};

@visibleForTesting
Map<String, String> get loadedEnv => _env;
Copy link

Choose a reason for hiding this comment

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

Making the loaded environment Map fully available (even if just read only) would help to not have to update any existing types in apps.
Since Platform.environment is a plain old Map<String, String> It would be preferred if we can get access to the raw map

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.

3 participants