fix: catch EnvMissingError and send error to reporter when no default value is provided in dev

[petercpwong]

https://github.com/af/envalid/blob/292fb133f8423be5d2aa346198e71c9d85cf2697/src/core.ts#L78-L80

When no default value is provided in dev mode, the EnvMissingError that is thrown is not subsequently handled and passed to the reporter. As a consequence, a cryptic error is thrown instead of having the reporter output a formatted error.

To reproduce:

import { cleanEnv, testOnly } from 'envalid';

cleanEnv({ NODE_ENV: 'development' }, { FOO: str({ devDefault: testOnly('sup') }) });

This PR fixes the aforementioned bug and also adds the necessary tests.

[af]

Thanks for the PR, especially the tests and repro case 💯

I verified that this is a bug and your PR fixes it 👍 I'm just wondering if you can think of a way to handle this case without expanding the try block so widely though?

[petercpwong]

I'm just wondering if you can think of a way to handle this case without expanding the try block so widely though?

Perhaps, we could append to the errors array instead of throwing an error? But that makes the code a tad less readable.

[af]

Hi, I finally got a chance to look in a bit more depth, I think there's a simpler solution. Rather than the big try/catch, just assigning to the errors object when we hit this case does the trick:

      if (usingDevDefault) {
        cleanedEnv[k] = spec.devDefault

        if (isTestOnlySymbol(spec.devDefault) && rawNodeEnv != 'test') {
          // this is the new line:
          errors[k] = new EnvMissingError(formatSpecDescription(spec))
        }

        continue
      }

The testOnly tests in basics.test.ts might need an update, since they currently fall back to the default reporter that calls process.exit– but creating a new reporter in the tests that just throws any provided errors does the trick.

[petercpwong]

If we just simply assign to the errors object instead of throwing an error, then we will have to duplicate the error handling code inside the catch block:

    } catch (err) {
      if (options?.reporter === null) throw err
      if (err instanceof Error) errors[k] = err
    }

[melalj]

Is there a plan to merge this PR? It seems that the first EnvMissingError is not caught, and we cannot use the reporter if NODE_ENV is not set to production.

[af]

@melalj thanks for the ping and sorry @petercpwong for letting this sit so long! I just released 8.1.0-beta.2 which includes this change