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

Change type definitions for env #83

Merged
merged 1 commit into from May 11, 2018

Conversation

Projects
None yet
3 participants
@neezer
Contributor

neezer commented May 9, 2018

closes #82

Presently have a failing test with this change, though:

Semantic: ~/Code/neezer/envalid/node_modules/@types/node/index.d.ts (6202,55): Cannot find name 'Map'.

@af would love a little help with that one.

@af

This comment has been minimized.

Show comment
Hide comment
@af

af May 11, 2018

Owner

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

Owner

af commented May 11, 2018

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

@af

This comment has been minimized.

Show comment
Hide comment
@af

af May 11, 2018

Owner

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

Owner

af commented May 11, 2018

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

@af

This comment has been minimized.

Show comment
Hide comment
@af

af May 11, 2018

Owner

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.

Owner

af commented May 11, 2018

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.

@neezer

This comment has been minimized.

Show comment
Hide comment
@neezer

neezer May 11, 2018

Contributor

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

Contributor

neezer commented May 11, 2018

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

@af af merged commit 639ab17 into af:master May 11, 2018

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@af

This comment has been minimized.

Show comment
Hide comment
@af

af May 11, 2018

Owner

Nice, thanks!

Owner

af commented May 11, 2018

Nice, thanks!

@neezer neezer deleted the neezer:process-type-change branch May 11, 2018

@lostfictions

This comment has been minimized.

Show comment
Hide comment
@lostfictions

lostfictions May 16, 2018

Contributor

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.

Contributor

lostfictions commented May 16, 2018

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.

@lostfictions

This comment has been minimized.

Show comment
Hide comment
@lostfictions

lostfictions May 16, 2018

Contributor

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
Contributor

lostfictions commented May 16, 2018

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment