@uppy/companion: merge config before validating

[andyroyle]

Running validateConfig can throw errors and stop the application from starting. If you're using environment variables to pass configuration then these need to be merged in before attempting to validate the configuration.

A concrete example of why this is needed is if you're setting COMPANION_UPLOAD_URLS when running NODE_ENV=production the application will fail to start because validateConfig requires that uploadUrls is specified.

[mifi]

If you're using environment variables to pass configuration then these need to be merged in before attempting to validate the configuration

in this PR you call validateConfig after merging defaultOptions with optionsArg. Prior to this PR, we call validateConfig only on optionsArg. it's not obvious to me why this change will make a difference because afaik COMPANION_UPLOAD_URLS (uploadUrls) gets validated the same way in both cases. defaultOptions doesn't even contain uploadUrls

[andyroyle]

it's not obvious to me why this change will make a difference because afaik COMPANION_UPLOAD_URLS (uploadUrls) gets validated the same way in both cases

When running with NODE_ENV=production, an empty uploadUrls config option prevents the service from starting (the validateConfig method throws an error). If you are setting uploadUrls using the environment variables and these get merged in after calling validateConfig, then uploadUrls will always be empty and validateConfig will throw an error.

[mifi]

still not sure i understand. if I run companion on main with NODE_ENV=production, it crashes as expected:

NODE_ENV=production yarn dev:with-companion
[nodemon] 2.0.19
[nodemon] to restart at any time, enter `rs`
[nodemon] watching path(s): packages/@uppy/companion/src/**/*
[nodemon] watching extensions: js,mjs,json
[nodemon] starting `node -r dotenv/config ./packages/@uppy/companion/src/standalone/start-server.js`
uppy/packages/@uppy/companion/src/config/companion.js:101
    if (process.env.NODE_ENV === 'production') throw new Error('uploadUrls is required')
                                               ^

Error: uploadUrls is required

If I run it with the env variable COMPANION_UPLOAD_URLS it seems to start correctly:

NODE_ENV=production COMPANION_UPLOAD_URLS=http://google.com yarn dev:with-companion
[nodemon] 2.0.19
[nodemon] to restart at any time, enter `rs`
[nodemon] watching path(s): packages/@uppy/companion/src/**/*
[nodemon] watching extensions: js,mjs,json
[nodemon] starting `node -r dotenv/config ./packages/@uppy/companion/src/standalone/start-server.js`
companion: 2024-01-31T08:51:16.502Z [info] jobs.cleanup.start starting clean up job
companion: 2024-01-31T08:51:16.523Z [info]  Welcome to Companion! v4.12.0
companion: 2024-01-31T08:51:16.523Z [info]  Listening on http://localhost:3020

If you are setting uploadUrls using the environment variables and these get merged in after calling validateConfig

env variables are merged in before module.exports.app is even called. it's merged in getCompanionOptions

uppy/packages/@uppy/companion/src/standalone/index.js

Line 21 in c51032c

const companionOptions = getCompanionOptions(inputCompanionOptions)

uppy/packages/@uppy/companion/src/standalone/helper.js

Line 224 in c51032c

exports.getCompanionOptions = (options = {}) => {

[mifi]

as I don't understand the benefit of this change and it only introduces a risk of bugs when changing the order, i'm going to close this for now