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

fix: load .env before config (proposal) #50

Merged
merged 7 commits into from
May 23, 2024

Conversation

ExampleWasTaken
Copy link
Contributor

@ExampleWasTaken ExampleWasTaken commented May 9, 2024

Description

This PR proposes a change to how the .env file is loaded.

The Problem

At the moment the .env file is loaded after the config. However, node-config relies on the env var NODE_CONFIG_ENV to determine what config to load. .env being loaded after the config, means you need to manually define NODE_CONFIG_ENV before running npm run dev. This defeats the purpose of having a .env file, which is why I propose this change.

The Fix

By loading .env before the config, we get rid of the requirement to define the NODE_CONFIG_ENV manually before running the application.
In fact, this PR follows the officially suggested best practice by dotenv and loads .env as early as possible, thereby removing the need of any environment variable to be defined manually prior to application start.

Test Results

As far I could test it, this works. However, I think it is a good idea to test it specifically against the official staging setup to make sure things like db access and 3rd-party APIs in fact work.

Additional Information

Changelog is missing because this is just a proposal for now and having the check fail prevents merging this PR with a wrong date in the changelog.

Discord Username

examplewastaken

Copy link
Collaborator

@pdellaert pdellaert left a comment

Choose a reason for hiding this comment

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

Don't forget CHANGELOG :)

@benw202 benw202 dismissed pdellaert’s stale review May 23, 2024 09:13

CHANGELOG added

@benw202 benw202 merged commit cd3460f into flybywiresim:staging May 23, 2024
3 checks passed
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.

None yet

3 participants