Allow cache.set(x, undefined) to delete the cached item

[cheap-glitch]

This is a very basic fix for the issue #25 discovered in refined-github/refined-github#3589.

Fixes #25

[fregante]

Thanks for the PR! This is a breaking change and also something that I had initially specifically avoided doing, so I'll probably wait just in case there's a reason not to do it.

[cheap-glitch]

Thanks for the PR! This is a breaking change and also something that I had initially specifically avoided doing, so I'll probably wait just in case there's a reason not to do it.

Maybe because it means cache.set('myKey') actually unsets the key instead of setting it, which can arguably be confusing?

[fregante]

I think the reasoning was that undefined could not be saved, because undefined is meant as "missing", even though .set is very well capable of storing undefined because it wraps it in an object first.

But also, yes, both .set('a') and .set('a', undefined) deleting 'a' are kinda weird.

I guess the former more than the latter. The first could throw an error though, at least to avoid using .set('a') by mistake instead of .get('a')

[fregante]

Can you add a arguments.length check so that it throws a TypeError when you call .set('a')?

[cheap-glitch]

Can you add a arguments.length check so that it throws a TypeError when you call .set('a')?

Done. Also added a test to cover the new case.

[fregante]

Thanks!

I'm releasing this as a minor after all. The behavior it changes isn't that breaking. Passing undefined was a no-op and made no sense in the first place.

[fregante]

I think the only issue here is that undefined isn't accepted by the types

Suggested change

	async function set<TValue extends Value>(key: string, value: TValue, maxAge: TimeDescriptor = {days: 30}): Promise<TValue> {
	async function set<TValue extends Value>(key: string, value: TValue | undefined, maxAge: TimeDescriptor = {days: 30}): Promise<TValue> {

I released a prerelease 4.3.0-0 if you want to try it in RGH in the original issue that discussed this