chore: refactor environment variables

[DylanVann]

Description

This is refactors environment variables so that all validation of them is done in one location (@app/config/src/index.ts).

This is only used by the server package in this PR, if accepted a followup can change the usage in other packages as well.

Performance impact

In the Express docs:

If you need to write environment-specific code, you can check the value of NODE_ENV with process.env.NODE_ENV. Be aware that checking the value of any environment variable incurs a performance penalty, and so should be done sparingly.

Some notes about this in relation to React SSR.

I haven't profiled as I don't think it would make a big difference, but that makes some intuitive sense.

Security impact

Developers need to be careful not to import secrets from @app/config in client code, or their secrets could get leaked, I believe Next.js might prevent this though, by not passing through environment variables to the browser. I can check this

Checklist

• [x] My code matches the project's code style and yarn lint:fix passes.
• [x] I've added tests for the new feature, and yarn test passes.
• [x] I have detailed the new feature in the relevant documentation.
• [x] I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
• [x] If this is a breaking change I've explained why.

[DylanVann]

I don't know that much about the docker setup. A hint about how to make sure the docker tests get SECRET and JWT_SECRET (which are now validated/required) would be appreciated 😄

Doesn't the docker version currently need the SECRET to function correctly? The results seem to be saying that it is not defined.

[benjie]

This approach might not be the best since it enforces variables whenever process.env would have been referenced, this is what is causing the Docker issues. The docker image should be able to build just fine without the SECRET being provided - the SECRET should only be required at runtime (whereas other variables like NODE_ENV are required at build time). Switching it to an "on demand" model where the first time an envvar is referenced it's validated and cached might be a better approach.

Another concern is that it references all envvars in any place any of them are referenced, whereas the previous code only references the required envvars in each place (so if you just run the worker you only read the envvars relevant to the worker). This may or may not have an effect on webpacking your server/etc. It will have an effect if you only expose the envvars each service needs to that service (e.g. the server has different envvar requirements to the worker, in an advanced deployment you might like to separate these out but this would prevent you doing that).

[DylanVann]

Thank you for the feedback! I'll take a second attempt at this soon.

I think you're right that this ties too many things together. The different components should only validate the environment variables they need.

[benjie]

[semi-automated message] Hi, there has been no activity in this issue for a while so I'm closing it to keep the issues/pull requests manageable. If this is still an issue, please re-open with additional details.