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

Feature request: Safe getter. #43

Closed
einnjo opened this issue Aug 10, 2017 · 8 comments
Closed

Feature request: Safe getter. #43

einnjo opened this issue Aug 10, 2017 · 8 comments

Comments

@einnjo
Copy link

einnjo commented Aug 10, 2017

While envalid is great for validating and parsing the variables from the environment, It's still possible to be affected by typos:

const env = cleanEnv(process.env, {HANDKERCHIEF: str()});

env.HANKERCHIEF; // Oops, misspelled.

It would be great to have a safeguard against this type of problem built into the library.

The impementation can be as trivial as:

function get(key) {

    if (env.hasOwnProperty[key]) {
        return env[key];
    }

    throw new Error('Invalid config key: ' + key);
}

Also possible are more fancy get functions. I have used something like this in the past:

[email protected]
AUTH_EMAILS_CHARSET=utf-8
AUTH_TOKENEXPSECONDS=3600
env.get('auth.emails.from') // key is transformed to uppercase and . -> _ before check.
@af
Copy link
Owner

af commented Aug 10, 2017

If you misspell the key in your config, you're going to get a validation error by default, right? Unless the env var is also misspelled, which seems unlikely.

As for the second part, check out the transformer option (I replied to your other issue suggesting it just as you posted this :). I think it'll give you the flexibility to do what you're looking for.

@einnjo
Copy link
Author

einnjo commented Aug 10, 2017

If you misspell the key in your config, you're going to get a validation error by default, right? Unless the env var is also misspelled, which seems unlikely.

Currently a validation error will only be thrown if the variable is mispelled inside the cleanEnv definition. If the variable is misspelled throughout the usage inside the app use(env.misspelled), no error will be thrown and that's what worries me.

As for the second part, check out the transformer option (I replied to your other issue suggesting it just as you posted this :). I think it'll give you the flexibility to do what you're looking for.

Absolutely, I retract the second part entirely.

@af
Copy link
Owner

af commented Aug 10, 2017

Ah I see, I misunderstood the question. Typos in the app could definitely be a source of errors and confusion.

I'm not sure about baking a solution into envalid at this time, but a good approach if you want to throw would be to wrap the cleaned env object in an ES6 proxy. Then you can throw an error (using the get trap) if the desired key isn't present on the cleaned object.

@einnjo
Copy link
Author

einnjo commented Aug 10, 2017

Yep, it's hard to make typos in cleanEnv since you only type it in one place, but var usage is spread everywhere and constantly changes so it's easier to make a typo there.

It's so easy to implement that I debated whether to open up an issue. But I figured that it could help other people from making the mistakes I did, I also thought that a library that dealt with validating the environment could also validate references to it.

I'll look into the proxies, I'm currently doing this on all of my projects:

// config.js
const envalid = require('envalid');
const { str, email, bool, num, url } = envalid;

const env = envalid.cleanEnv(
    process.env,
    {
        // ... Declare config variables.
    }
);

function get(key) {
    if (env.hasOwnProperty(key)) {
        return env[key];
    }

    throw new Error('Missing config key: ' + key);
}

module.exports = { get };

// Elsewhere...
const Config = require('./config');

getApiResponse(Config.get('API_ERL')); // Throws, should be URL

Anyways, thanks for the great libary and fast responses 👍

@af
Copy link
Owner

af commented Aug 10, 2017

Yeah it's a very valid point. I'll mull it over for a bit and consider adding a proxy wrapper by default. Thanks for the feedback!

@SimenB
Copy link
Collaborator

SimenB commented Aug 11, 2017

We use a proxy at work as well.

function makeImmutable (mutable) {
    return new Proxy(mutable, {
        get (target, name) {
            // These two checks are needed because calling console.log on a
            // proxy that throws crashes the entire process. This whitelists
            // the necessary properties for `console.log(config)` to work.
            if (['inspect', Symbol.toStringTag].includes(name)) {
                return mutable[name];
            }
            if (name.toString() === 'Symbol(util.inspect.custom)') {
                return mutable[name];
            }

            const val = mutable[name];
            if (val === undefined) {
                throw new Error(`Config key not found: ${name}`);
            } else {
                return val;
            }
        },

        set (name) {
            throw new Error(`Config values can not be changed: ${name}`);
        },
    });
}

(should probably use hasOwnProperty )

@af
Copy link
Owner

af commented Aug 11, 2017

Cool, thanks for the example @SimenB. Any other gotchas you're aware of that would preclude including a proxy wrapper by default when the strict option is set?

@SimenB
Copy link
Collaborator

SimenB commented Aug 11, 2017

Nah. We've been using that code in production for close to a year without issues

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

No branches or pull requests

3 participants