Plural Keyv.get([]) types are incorrect, or Store.getMany() returns incorrect type

[trevor-scheer]

Additional context: apollographql/apollo-utils#260

Describe the bug
The "plural" Keyv.get([]) overload is expected to return Promise<Array<V | undefined>> based on the typings. However, you can see in my PR that when a Store.getMany() returns undefined, then undefined is returned by Keyv.get([]).

Should Store.getMany() be required to always return an array (I think no, undefined seems semantically useful in the error case, maybe)? Or should the Keyv.get([]) types be updated to include the possibility of undefined so we know to handle that case?

How To Reproduce (best to provide workable code or tests!)
Reproduction available in the linked PR.
The initial commit demonstrating bad behavior might be more interesting:
apollographql/apollo-utils@6062106

[jaredwray]

Hi @trevor-scheer - would love to see a code sample on how to reproduce this (it can be really simple) and also what you would like to see. Thanks!

[trevor-scheer]

Sure thing. Here's a codesandbox where my runtime is in conflict with the types (there's a sneaky undefined case).

I think that undefined is semantically useful for a Store to convey an error (and at least the Redis store currently exhibits that behavior) in the getMany case (as well as the get case - which is non-problematic).

I'll open a PR with my suggestion. I think just making the overloaded Keyv.get([]) method include undefined as a possible return type would be the ideal outcome. No behavior change, just a typings "bug fix".

[jaredwray]

Hi @trevor-scheer 👋

Sorry for the delay on this as I am doing the research since this could be considered a major breaking change. I will be reviewing it more this week as the initial plan on getMany was to always return an Array no matter what. The reason for this is that each item inside that array could be undefined but not the array itself.

It might make more sense to always return an array but fix the Store to convey this change or make sure at Keyv to always return the array even if the store returns undefined we would replace it with an []. Thoughts?

[trevor-scheer]

@jaredwray no worries on the timing.

Right now the typings aren't inline with the runtime. Aligning the types to the reality, while "introducing" new TS errors for users on upgrade, would help them to address currently invalid assumptions in their code - i.e. it would surface existing bugs. For that reason I'd consider the PR I've opened to be a bug fix, since there's a bug in the types.

Changing the behavior of either Keyv or Store implementations would be a change in the runtime. So if getMany (or plural Keyv.get) always returning an array is a desired end state, I think that change should be part of a major version bump.

In any case, I think I've fully convinced myself that my PR should land as a patch. I'd be happy to put some more thought into the future API with you if you're interested in any runtime changes - though I think the current state is reasonable.

[jaredwray]

@trevor-scheer - just getting back to this and do agree that this should be a patch. Do you want to update your PR to a working state and we will get this merged in?

[jaredwray]

@trevor-scheer - I just pulled this down and it looks like changing the Keyv.get([]) would break more than changing the Store.getMany(). Some of these projects are doing millions of downloads so I am thinking the easier implementation to do is to change the Store.getMany() and remove the option of undefined.

Just tested it on a couple major projects such as GOT and Apollo and it will cause a big break. What would it take to move to being based on the standard instead?

[trevor-scheer]

@jaredwray I think there might be a bit of a misunderstanding here or we actually disagree on whether or not this is a breaking change.

I don't see how this could be a big break for anyone consuming this package, given that it's only a typings change. Yes - for typescript consumers, there will be new errors upon upgrading keyv that force them to handle the undefined case. In its current form, the undefined case can happen at runtime, so the types are hiding a bug. This change encourages consumers to handle the undefined case correctly via typings.

The alternative of changing Store.getMany() is an actual runtime change that breaks existing behavior and would require a major version bump, right? If you do prefer to go that route that's an option of course, but I think what I have is simpler since it preserves existing behavior.

[glasser]

Yeah, it seems to me that if the plan for fixing this is to change Store.getMany() to be declared to never return undefined, then that's a major version change (and will require code changes to @keyv/redis). Is that the proposal here? Seems reasonable if so; it's a bit of a shame to leave today's major version in a state where the typings don't actually describe the behavior, but if that's because people should just upgrade to the new major version then that's OK I guess.

[mmkal]

Wouldn't it be possible to just declare this a runtime bug, not a types bug, and make a small change to default to [] - maybe even in just one place (here?) - and call it a bug fix? The stores which implement getMany(...) can keep returning undefined for now, but keyv could default to [].

That way the types won't break the many millions of dependents (via got), and it's very unlikely the runtime would either. An empty array is generally a friendly thing to return a user who is expecting array. Maybe some people will be doing null-checks that become unnecessary. The type change in #728, on the other hand, will force countless people to do a null-check or face a build error.

I don't know this repo, but maybe this line:

keyv/packages/keyv/src/index.js

Line 148 in ed9f487

.then(() => isArray ? store.getMany(keyPrefixed) : store.get(keyPrefixed))

could be changed something like:

		return Promise.resolve()
-			.then(() => isArray ? store.getMany(keyPrefixed) : store.get(keyPrefixed))
+			.then(() => isArray ? store.getMany(keyPrefixed).then(result => result || []) : store.get(keyPrefixed))
			.then(data => (typeof data === 'string') ? this.opts.deserialize(data) : (this.opts.compression ? this.opts.deserialize(data) : data))

[jaredwray]

Hi @trevor-scheer - I am going to close this in favor of the work we are doing on Keyv v5 as that will have a more defined type. Please follow along here: #868

[trevor-scheer]

Thanks @jaredwray, looking forward to v5!