Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add requiredWhen to validator spec #81

Closed
wants to merge 1 commit into from

Conversation

neezer
Copy link
Contributor

@neezer neezer commented May 8, 2018

closes #78

* A function that receives the full raw environment and returns a boolean
* indicating if this field is required or optional.
*/
requiredWhen?: (env: any) => boolean
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@neezer neezer May 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

😉

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really elegant, thanks!

@neezer
Copy link
Contributor Author

neezer commented May 9, 2018

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
Copy link
Contributor Author

neezer commented May 9, 2018

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
Copy link
Owner

af commented May 9, 2018

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
Copy link
Collaborator

SimenB commented May 9, 2018

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

@af
Copy link
Owner

af commented May 11, 2018

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

@neezer
Copy link
Contributor Author

neezer commented May 11, 2018

@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
Copy link
Owner

af commented May 11, 2018

@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?

@DylanVann DylanVann mentioned this pull request Jan 30, 2019
@af
Copy link
Owner

af commented Nov 1, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditional required env?
3 participants