Add TypeScript types by eugene1g · Pull Request #201 · isaacs/node-lru-cache

eugene1g · 2022-03-02T20:48:26Z

This adds TypeScript definitions for this project based on the current public API of LRUCache.

This PR includes tests to ensure the types are defined "correctly", but "correctly" means they are TS compatible and describe how the public API works. However, as the original source code is in JS (and not TypeScript), as the project adds/removes features, it will still be necessary to update the types manually. This should be fairly easy to do, but it's a manual step all the same.

If you would consider migrating the entire project to TS, that could be done relatively easily as well.

Closes #195

package.json

test-d/lru-cache.test-d.ts

types.d.ts

…ability with what's published on DefinitelyTyped

isaacs

A few possible nits, but this looks mostly accurate and complete. I'll try to get it pulled in this weekend.

package.json

types.d.ts

isaacs · 2022-03-03T15:38:38Z

types.d.ts

+   * @since 7.3.0
+   * @default false
+   */
+  noDisposeOnSet?: boolean | undefined;


Do you need | undefined if it's optional?

I've seen developers conditionally define config values based on some other flags, like so -

let disposeValue; if(condition1) disposeValue = true; if(condition2) disposeValue = false; const cache = new LRUCache({ noDisposeOnSet: disposeValue });

The semantic meaning is "I might have an opinion on that flag, or might be ok to accept the default by passing undefined". So in the type system, even for optional values it seems like good DX to still allow people inject undefined values which works the same as omitting that setting.

isaacs · 2022-03-03T15:40:59Z

types.d.ts

+   * Find a value for which the supplied fn method returns a truthy value, similar to Array.find().
+   */
+  public find<T = Value>(
+    fn: (value: Value, key: Key, cache: this) => boolean | undefined | void,


Really this can return any, but if you're going to specify a type, maybe boolean would be best?

IMO, predicate functions often have problematic logic that we can prevent by using strict return types -

void help for use-cases when the developer returns only when they find a match, for example -

const importantStuff = cache.find( (value, key) => { if(typeof value === "string" && value.includes("important1")) return true; // no return by default, therefore the function could have a `void` return type });

boolean forces the developer to think about false positives when matching for values that could be falsey in JS, for example -

const specialDeals = cache.find( (value, key) => value.discountRate); // this would miss records where value.discountRate === 0, and the developer would be better off if they have to express it as a boolean, e.g. "typeof value.discountRate === 'number'" or "value.discountRate>0"

isaacs · 2022-03-03T15:41:27Z

types.d.ts

+   *  - `sizeCalculation`
+   *  - `noDisposeOnSet`: prevent calling a dispose function in the case of overwrites.
+   */
+  public set(key: Key, value: Value, options?: SetOptions<Key, Value> | undefined): this;


Do you need | undefined here?

Don't need this part here (| undefined), but it's technically allowed to call this method as cache.set('key1', 42, undefined) so I left it in.

Ohh, I see, so the ? means you can omit it, and | undefined means you can send undefined?

test-d/lru-cache.test-d.ts

isaacs · 2022-03-04T00:23:49Z

test-d/lru-cache.test-d.ts

+
+// It's ok to return bool or nothing
+expectType<TRecord|undefined>(userCache.find((val,key,cache)=>{
+    if(Math.random()<0.5) return false;


Note: this will never find anything, since it doesn't return any truthy values.

True! It doesn't matter for testing the types (the test is that the function doesn't have a return path), but it shows a meaningless example and worth tweaking.

Somewhat an aside here, I do prefer to avoid using random values in tests, since it means that coverage is nondeterministic. Eg, in the if (Math.random() < 0.5) return true case, it might not ever run the code path where it returns undefined. But if the test is just verifying that it compiles properly by finding the possible return types, maybe that's fine. I don't know how this actually works 😅

isaacs · 2022-03-04T03:02:00Z

Ok, so... is this actually validating that the types in the .d.ts file match the code in the index.js?

If I add this:

diff --git a/index.js b/index.js
index e9b2f37..e9501cd 100644
--- a/index.js
+++ b/index.js
@@ -453,7 +453,7 @@ class LRUCache {
     }
   }
 
-  get (k, {
+  get (k, blorg, {
     allowStale = this.allowStale,
     updateAgeOnGet = this.updateAgeOnGet,
   } = {}) {

then run npx tsd, it still passes.

Is the test just verifying the types against themselves? If it's not going to break when the JS changes, then it seems like I'm just signing up to maintain something without tests.

eugene1g · 2022-03-04T15:29:24Z

Ok, so... is this actually validating that the types in the .d.ts file match the code in the index.js?
Is the test just verifying the types against themselves? If it's not going to break when the JS changes, then it seems like I'm just signing up to maintain something without tests.

Unfortunately, yes. In this iteration, the new TS types are just a form of machine-readable docs. Unlike the docs in the README, TS types can have tests to verify that the documentation is syntactically valid, and include typical use-cases to ensure those common usage patterns have correct hints - but not that the types accurately represent the "black-box" JS code - if this were possible, we wouldn't need TypeScript the language :)

As long as the main code is written in JS, there is no way to guarantee that the TS types (that are manually written) correctly represent the behavior of the latest JS code. For example, you can decide that one parameter is now mandatory, and enforce it with if(!ops.ttl) throw new Error("TTL is a must");. There is no way to automatically update TS types to reflect this change - you'll need to update the README and the TS types.

Recently, more JS projects choose to migrate to TypeScript, as it's a relatively simple process. Once your main code is in TypeScript, then all types are generated automatically from the source code, and thus never can drift from the actual implementation.

eugene1g · 2022-03-04T15:32:19Z

If you're open to it, I can take a pass at migrating the main codebase to TS - I'd make the least amount of changes only required for TS (no refactoring/reformatting code, etc) to keep a clean PR.

isaacs · 2022-03-04T18:08:28Z

I am not open to writing the code in TS, sorry. Too much care is taken to ensure that the code is as performant as it can be, and that's just not possible with a transpiled language.

For example, you can decide that one parameter is now mandatory, and enforce it with if(!ops.ttl) throw new Error("TTL is a must");. There is no way to automatically update TS types to reflect this change - you'll need to update the README and the TS types.

If a parameter is made mandatory, then I would think that a TS test verifying that all marked-optional fields can be omitted, would begin to fail. Likewise if a new parameter is added, a field removed, etc.

I don't expect to have the types automatically updated when the JS code changes (though of course that would be nice). Only that there is a test that fails when that happens, so that I can notice, and know what to fix. If that's not possible, then I would prefer that these type definitions live in DefinitelyTyped, or some other place so it's clear that I'm not maintaining them (because I won't be, if there are no tests to tell me when they are broken).

isaacs · 2022-03-04T18:10:29Z

Too much care is taken to ensure that the code is as performant as it can be, and that's just not possible with a transpiled language.

Also, if I'm being honest, I am just much more familiar with JavaScript than TypeScript, and find TS far more annoying than useful to work with. If it reliably produced more performant code, I'd swallow my preferences and do it anyway, but it doesn't. It produces type-safe code, which is a different priority. There's a reason this module exposes all its internals and just says "do not touch (or else)" -- it's much faster that way.

eugene1g · 2022-03-04T18:23:03Z

If a parameter is made mandatory, then I would think that a TS test verifying that all marked-optional fields can be omitted, would begin to fail. Likewise if a new parameter is added, a field removed, etc.

Just to confirm what level of assurance you are looking for -

If you make a change to the API in JS, you are "ok" to manually update the TS definitions (in types.d.ts). You do not love this, but it's similar to having to update documentation, so you might be OK with it. But, what you need to have automated, is a test that should fail at this point because the parameter was assumed to be optional before. Then you'll update the test accordingly, and publish a release. Right? If so, this is a matter of increasing the test coverage for TS types, and I can do that.
As a more concrete example, recently you added the option noUpdateTTL to set. If you had TS types in the repository, nothing would fail as nothing was written for that new option. Would you be OK to manually add the type definition for the new flag into types.d.ts, then add another test (via copy&paste in test-d/lru-cache.test-d.ts)?

isaacs · 2022-03-04T19:24:33Z

As a more concrete example, recently you added the option noUpdateTTL to set. If you had TS types in the repository, nothing would fail as nothing was written for that new option. Would you be OK to manually add the type definition for the new flag into types.d.ts, then add another test (via copy&paste in test-d/lru-cache.test-d.ts)?

Honestly for that kind of thing, I'll probably not think of it until someone complains, if they ever do.

I know it seems pedantic and a little annoying, but I just don't want to be in a situation where I'm making promises I can't keep. Better for users to get the types elsewhere in that case, so it's clear that it's not "officially" part of the module. (Even then, I often get TS users complaining on various projects that the types are inaccurate, when I had no idea that types even existed. I feel bad for them, but also, I can't really help them.)

eugene1g · 2022-03-04T19:27:08Z

Gotcha, thanks for evaluating - I probably should move this to DefinitelyTyped.

isaacs · 2022-03-04T20:28:49Z

Gotcha, thanks for evaluating - I probably should move this to DefinitelyTyped.

Thanks for understanding. I'm definitely down to review and help make it as accurate as possible, I just can't sign up for ongoing maintenance without tests. I know myself too well 😅

eugene1g · 2022-03-06T15:10:47Z

Continuing this at DefinitelyTyped/DefinitelyTyped#59162 , thanks @isaacs !

feat: add TypeScript types

a9781e6