Add requiredWhen to validator spec

[neezer]

closes #78

[neezer]

Out of curiosity, why any instead of NodeJS.ProcessEnv? Speaking for the whole file here...

[SimenB]

Can you then pass your own object? I don't use (or know) TS, but at least for tests it's useful to say envalid.cleanEnv({someFake: 'env'}, bla)

[neezer]

Looks like NodeJS.ProcessEnv will accept any indexable object where the key is a string and the value is either a string or undefined.

declare namespace NodeJS {
        // ...
        export interface ProcessEnv {
                [key: string]: string | undefined;
        }
        // ...
}

So your example of { someFake: 'env' } would compile, but

• { someFake: null }
• { someFake: 42 }
• { someFake: false }
• { someFake: [1, 2, 3] }
• { someFake: { really: 'fake' } }
• { someFake: fake => anotherFake }
• { someFake: Symbol('gimli') }

... would not. This makes sense to me since env vars are read in to process.env as strings.

I think you'd probably get away with just changing envalid.d.ts, if you wanted to.

Should make this change in a different PR if y'all wanted it; just seemed like a good place to ask.

😉

[SimenB]

We've explicitly added support for { someFake: false } etc for testing purposes, so they should keep working...

But yeah, feel free to open up a separate issue!

[SimenB]

Really elegant, thanks!

[neezer]

Our conversation in #82 has made me realize this PR has a flaw, particularly when it comes to boolean values: the bool() validator treats 't', 'true', and '1' as truthy and 'f', 'false', and '0' as falsy, however the raw—not parsed—environment is passed into requiredWhen.

This means that env => !!env.USE_TLS will evaluate to true in all of the above cases, including the last three that should evaluate to false.

You could as a user implement your own check about equality to "falsy" strings, but this feels like something envalid should be smart enough to do for you using the same internal rules it's already using.

---

❌ I don't think this PR is ready for merge. Let me see what I can do about the above issue.

[neezer]

To clarify my last comment, the failing test not included in this PR is this:

- // FOO not required when REQUIRE_FOO is false
+ // FOO not required when REQUIRE_FOO is absent
  assert.deepEqual(cleanEnv({}, spec, makeSilent), { REQUIRE_FOO: false })
  
+ // the test below currently fails when it should succeed
+ 
+ // FOO not required when REQUIRE_FOO is false
+ assert.deepEqual(cleanEnv({ REQUIRE_FOO: 'false' }, spec, makeSilent), { REQUIRE_FOO: false })

[af]

Ah yeah, that's a great point. I can't think of a workaround off the top of my head. Obviously it'd be nice to pass the cleaned env into the requiredWhen cb, but the cleaned env would depend on the requiredWhen validation happening first

[SimenB]

Anything stopping us from parsing and cleaning all variables before validating their presence?

[af]

@SimenB Yeah, that's probably the way to go, it would just require a non-trivial refactor first

[neezer]

@SimenB That's what I've done in my local refactor, but I moved a lot of stuff around to do it. Mostly extracting functions out of cleanEnv, though: on the plus side, got rid of the ESLint complexity warnings. I was a bit worried y'all would think the diff to large for this feature—and I made some other changes orthogonal to this feature—so I didn't update the branch with the changes.

If you want to take a peek, you can find them on master...neezer:conditional-envs-2

Note: half of the diff is updates to test that pertain to my comment here.

A drawback to my approach in this branch is that you can only compare other ENV vars that are not using requiredWhen: so you couldn't requireWhen and target another requiredWhen var. This feels a bit non-intuitive to me, tbh. Thoughts?

[af]

@neezer I just took a quick look at that diff, but it looks promising. Will have to chew on the requireWhen restriction you mentioned though, that does seem like a potential footgun. Is there a way we could fail with a useful message if someone tries to chain requireWhen vars?

[af]

This is prettttty stale by now, but if anyone wants to pick this back up and re-submit, feel free