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

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

[DylanVann]

I don't really understand what this one was doing in the original code, but I think this functions the same.

[DylanVann]

This and the comment at the top of this file seem to have been resolved. Also // Foo below may have been resolved 🤔 .

[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.

[DylanVann]

A mini validation framework:

• required
• defaultNumber
• minLength
• parseInteger

Other options were using envalid, io-ts, etc. I think it's simple enough that isn't needed.

[benjie]

Interesting approach. I'm concerned about this being easily misused; could you move most of the new variables to a new path @app/config/server so that it makes it very obvious it should not be required in the client? (You'll may need to create a server.js file that just contains module.exports = require('./dist/server.js') so you can write a src/server.ts. I'm not ready to use the fancy package.json module mapping features yet.)

This isn't a full review.

[benjie]

I'm surprised the linter does not complain about unnecessary type definitions here (since they can be implied by the values).

[DylanVann]

I'd usually go for inferred but I wanted to ensure that people changing the validators didn't accidentally do something like change the exported type from string to string | undefined without making it explicit.

[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.