fix(types): fix return value of get() #52

boneskull · 2022-08-02T20:58:44Z

Also fix a handful of jsdoc problems
Upgrade TS so it understands jsdoc better
Enable typechecks
Upgrade package-lock.json to version 2 (npm v7+ seems to do this whether you want it to or not)

Hey @nzakas, I saw you announce this pkg a long while ago on Twitter, and I liked the API, so I thought I'd use it.

Currently, get() returns a type of string|undefined--but if a user provides a default value, the result is guaranteed to be a string.

With my changes, we can see that the value of guaranteed will be a string:

And here we see that the value of notGuaranteed is either string or undefined:

Also: if you don't have the time or inclination to maintain this package, I can offer to help out.

boneskull · 2022-08-02T21:02:33Z

FWIW tsc can generate the declarations from src/env.js instead of dist/env.js, which would certainly improve DX (since you'd get type warnings in the actual source).

nzakas

Hey thanks! I’m still maintaining this, it’s just mostly feature complete so there’s not a ton of commits.

I left a few questions about various TypeScript things I’m not familiar with. I really appreciate you taking the time to tighten up the typing, I just want to be sure I understand it. :)

src/env.js

nzakas · 2022-08-03T00:45:46Z

src/env.js

     *      environment variable is not found.
-     * @returns {string|undefined} The environment variable value if found or undefined if not.
+     * @returns {S extends string ? S : (string|S)} The environment variable value if found or undefined if not.


Also, how would I read this type?

This is in response to L77/79 as well.

I hope I'm explaining this OK, since I haven't really had to explain it before.

What we have here is a generic type. One way to use a generic is to change the return value of the function depending on the type of a parameter. In this case, we always expect the key parameter to be a string, but defaultValue can be a string or undefined if omitted. But given the logic of the function, the type of the return value depends on the value of defaultValue--if it's a string, we always get a string in return.

(This same sort of thing could be accomplished with method overloading in TS--which, of course, JS does not support, so we need to use generics.)

We define a generic in JS with a @template tag. @template X creates a generic named X. It can be of any type. @template {Error} Y creates a generic Y, which must "extend" Error. This means Y, in the case of Error, must be an Error, TypeError, ReferenceError, etc. We can also provide "defaults" for generics by using the syntax @template {Error} [Y=Error]. Often a generic type will require type parameters; e.g., in TS, const mySet: Set = new Set() would be invalid, because Set requires a type parameter. const mySet: Set<string> = new Set() would be correct (and if you ask TS to typecheck your JS, you'll get the same complaint; you need to explicitly tell TS it has a Set<string> by using @type {Set<string>}).

Here, we have @template [S=undefined]. The type of the defaultValue param is S (or undefined, since it's optional). The generic S allows us to use a ternary-lookin' conditional type, which is S extends string ? S : (string|S). It means "if S looks like a string, return a value of that type (string-ish), otherwise return preferably a string (if the env var is set) or (if the env var is unset) whatever S is (typically undefined).

The outcome is that TS's inference can easily tell if you call Env.get without a second parameter, and it deduces that the return type must be string|undefined. But if the second parameter is a string, then we must get a string return type.

Ah, thank you. Makes sense now. 👍

I think this change could also apply to the first() method, but I'm not using that one, so I didn't change it. If you want me to, I can do that as well.

Yeah, please go ahead with that change. 👍

This is done

nzakas · 2022-08-03T00:46:15Z

src/env.js

@@ -175,6 +179,10 @@ export class Env {
    get exists() {

        const existsProxy = new Proxy(this.source, {
+            /**
+             * @param {any} target 


Should this be object?

it shouldn't really be either object or any, it should be a generic T which extends NodeJS.ProcessEnv|Deno.Env, but it didn't matter for my purposes. we'd have to install some DefinitelyTyped modules to get that to work.

You can pass any object into the Env constructor to override the default. Deno.env isn’t the value we use for Deno, it’s Deno.env.toObject(). The only guarantee is that this.source will be an object whose keys are the variables you want access to. You can’t, for instance, use a number of string (which I think “any” allows?)

So the more I look at this, the worse it becomes. I spent awhile with it this morning and discovered that TS is bad at doing the sort of thing the @humanwhocodes/env package does. In other words, strongly typing this.source and the types of its keys will be a headache and require much verbosity and typecasting--at minimum. I also understand that using object is kind of fiddly in general, and really what TS wants is for any object to be strongly typed instead--otherwise any is appropriate, even though it doesn't obviously make sense.

Since we don't know what this.source will be, we'd need to go down a rabbit hole of generics and type guards to make all of it compile cleanly.

....

I've removed these docstrings on the Proxys and instead modified the two error helper functions to accept PropertyKey values, which is what Proxy expects the second parameter (key) to be.

You should also know that TS cannot infer that a function throws unless it itself throws. This means that calling out to a function which always throws is not recommended; TS cannot infer the calling function throws when the call is made. Anyhow, since I'm not gonna mess with that stuff, it doesn't really matter here.

In addition to this, I've made one last change which builds the declarations from src/env.js instead of dist/env.js, which makes it easier to see TS warnings/errors in the sources if you so chose (otherwise the output .d.ts file is roughly identical).

(FWIW, I haven't used Deno, and don't know what Deno.env.toObject() returns; I was not being literal about the type)

nzakas · 2022-08-03T00:46:52Z

src/env.js

@@ -175,6 +179,10 @@ export class Env {
    get exists() {

        const existsProxy = new Proxy(this.source, {
+            /**
+             * @param {any} target 
+             * @param {string} key 


If we want to be 100% correct, key can also be a symbol.

Yes, Proxy would accept a symbol key, but we can't, since this.source only has key strings in practice. I mean... maybe you could stuff a symbol in process.env but I'm not sure how you'd do that from bash 😄

Keep in mind that source can be passed directly into the Env constructor. It doesn’t have to be read from the environment.

nzakas · 2022-08-05T00:44:48Z

src/env.js

@@ -28,22 +30,22 @@ const defaultEnvSource = (() => {

 /**
 * Throws an error saying that the key was found.
- * @param {string} key The key to report as missing.
+ * @param {PropertyKey} key The key to report as missing.


Is PropertyKey defined by default in TS?

- Also fix a handful of jsdoc problems - Upgrade TS so it understands jsdoc better - Enable typechecks - build declarations from sources instead of build artifacts

nzakas

Thanks for this! I learned a lot.

nzakas · 2022-08-06T00:53:16Z

Forgot I didn’t set this repo up to automate releases, so I’ll publish this next week when I get around to fixing that.

nzakas reviewed Aug 3, 2022

View reviewed changes

boneskull force-pushed the boneskull/type-fix branch 2 times, most recently from 6b42710 to 7dce76e Compare August 4, 2022 20:07

nzakas reviewed Aug 5, 2022

View reviewed changes

fix(types): fix return value of get()

c1aa525

- Also fix a handful of jsdoc problems - Upgrade TS so it understands jsdoc better - Enable typechecks - build declarations from sources instead of build artifacts

boneskull force-pushed the boneskull/type-fix branch from 7dce76e to c1aa525 Compare August 5, 2022 19:30

nzakas approved these changes Aug 6, 2022

View reviewed changes

Merge branch 'master' into boneskull/type-fix

6cd7b50

nzakas approved these changes Aug 6, 2022

View reviewed changes

nzakas merged commit 6382172 into humanwhocodes:master Aug 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(types): fix return value of get() #52

fix(types): fix return value of get() #52

boneskull commented Aug 2, 2022

boneskull commented Aug 2, 2022

nzakas left a comment

nzakas Aug 3, 2022

boneskull Aug 3, 2022

nzakas Aug 4, 2022

boneskull Aug 4, 2022

nzakas Aug 5, 2022

boneskull Aug 5, 2022

nzakas Aug 3, 2022

boneskull Aug 3, 2022

nzakas Aug 4, 2022

boneskull Aug 4, 2022 •

edited

Loading

boneskull Aug 4, 2022

nzakas Aug 3, 2022

boneskull Aug 3, 2022

nzakas Aug 4, 2022

nzakas Aug 5, 2022

nzakas left a comment

nzakas commented Aug 6, 2022

fix(types): fix return value of get() #52

fix(types): fix return value of get() #52

Conversation

boneskull commented Aug 2, 2022

boneskull commented Aug 2, 2022

nzakas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boneskull Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

nzakas commented Aug 6, 2022

boneskull Aug 4, 2022 •

edited

Loading