@neezer Just for the record, which version of TypeScript and @types/node are installed?

Oh! Sorry, it's the CI job you're referring to. Looks like some of those deps are pretty stale, I'll see if updating them helps

OK, could you rebase off of latest master and see if the error is resolved? I pushed some updates, including the TS checker one, which seems to have been causing the error.

@af That seemed to have done the trick. Green now!

Nice, thanks!

Hey there, just noticed this change... this seems to significantly change typings for the worse for no benefit and additional maintenance cost? I read #82 but the justification doesn't really sense to me.

Firstly, changing the parameter type to NodeJS.ProcessEnv doesn't really seem to bring much benefit. Here's how NodeJS.ProcessEnv is typed:

export interface ProcessEnv {
        [key: string]: string | undefined;
}

(from the typings here)

That is indeed slightly more strict than any since it won't accept, say, primitives, but we could introduce the same thing by changing any to { [key: string]: string | undefined } ourselves. Unfortunately using Node typings for that one line means we're also introducing a hard dependency on @types/node for that -- which not only means a new dependency to keep up to date, but also injects typings for Node globals into the global namespace. I guess it's not a big deal, but it seems like an additional maintenance burden and a small footgun for nothing.

Much more problematic is this addition to the CleanEnv interface:

    readonly [key: string]: string | object | number | boolean | undefined

This seems like it'll break the typings for cleanEnv({ strict: true }) -- whereas before it would infer the strict return value, now it'll return an object where any property is valid. That seems like a significant downgrade to me.

Incidentally, I didn't really have time to set this up last PR I made, but it would maybe be a good idea to add a test in https://github.com/af/envalid/blob/master/tests/test_typescript.js that tries to compile an invalid TypeScript file and expects it to throw?

eg.

const strictEnv = cleanEnv(
    {},
    {
        foo: str({
            desc: 'description',
            default: ''
        })
    },
    { strict: true }
)

const x = strictEnv.nonsenseProperty

Why is this line introduced?

readonly [key: string]: string | object | number | boolean | undefined

I previously used this function to provide type-sound extraction of configurations:

interface AppEnvironment {
  DB: string;
  PORT: number;
  JWT_TOKEN: string;
}
type IEnvironment = AppEnvironment & envalid.CleanEnv;
public get<K extends keyof IEnvironment>(key: K): IEnvironment[K] {
  return this.envConfig[key];
}

But by adding that line of code, I can no longer get a list of property keys on what envalid.cleanEnv emits.

@coderfox yeah -- see my comment above. I've just stuck to the old version of envalid since this introduces soundness issues and my comment kinda got ignored... @af any chance of reverting or reconsidering this change?

It's a bit hard to fully evaluate the change as a non-TS user, but I'd love to see a PR that reverses the contested bits. @SimenB I think has more TS experience so he is probably in a better position to make the call on this one

Yeah, I think we should revise this. @lostfictions wanna send a PR fixing it? With the test you mentioned so we don't regress again.

Would be cool if we could infer the types of the variables from the validators removing the need for your extra interface as well

Could we just remove this line:

readonly [key: string]: string | object | number | boolean | undefined

... and introduce & NodeJS.ProcessEnv on non-strict return type?

If this is okay, then I can send a PR right now.

Thanks for the poke! Submitted #98.

@SimenB sorry, I don't use Painless and couldn't figure out an easy way to make falsifiable tests that only fail for a specific scenario, so in the end I didn't add it. And it we don't test for specific failures, the tests might not be effective at presenting regressions regardless... On further reflection, even if we could test for a specific error, depending on error messages from tsc matching up (say, one complaining about an unknown/invalid property on an object) across Typescript versions might be kind of brittle anyway, so it's not clear to me that it's a good idea.

For what it's worth I added an explanatory comment about why a certain type signature was the way it was instead :) Feel free to flag me to review TS definition changes in the future if that's helpful!