-
Notifications
You must be signed in to change notification settings - Fork 858
Update config-helper to ignore non-existent config.json #4896
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
Update config-helper to ignore non-existent config.json #4896
Conversation
Size Change: -7 B (0%) Total Size: 6.87 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for adding better logging 🚀 There's just an improvement we can do here.
server/src/config-helper.ts
Outdated
if (BASE_CONFIG) { | ||
console.log('Loading config: reading BASE_CONFIG ...') | ||
return BASE_CONFIG | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to check whether BASE_CONFIG
exists, because it's always set either with default values or environment variables. So we can we can shorten it here to:
if (BASE_CONFIG) { | |
console.log('Loading config: reading BASE_CONFIG ...') | |
return BASE_CONFIG | |
} | |
console.log('Loading config: reading BASE_CONFIG ...') | |
return BASE_CONFIG |
So regarding The config gets called a lot because it provides a lot information that is required at every step, e.g. the environment alone (i.e. sandbox, stage, prod) is probably needed in a lot of files. We also don't really want to log what gets called because we don't want to pop secrets into logs that are sent around the globe :) |
Confirming requested changes have been made re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the much needed update 👍
Pull Request Form
Type of Pull Request
This PR:
configuration-helper.ts
and no longer checks for aconfig.json
file, which was causing confusion with people setting up their own development environment, as it threw an error if it wasn't found, even though it wasn't required.loadedConfig
which no longer serves a purpose AFAICTconsole.log
I do have some questions about things that I don't understand in this file:
injectedConfig
works, or what purpose it serves?getConfig()
, but that would be helpful here.