Unexplained change to decode return type causing 1.0 migration difficulty

[benmccann]

Pre-1.0 we had:

decode?(value: string): string;

Now we have:

decode?: (str: string) => string | undefined;

This is blocking us from upgrading because it means we would potentially be passing undefined values to our users where we weren't before. It's very unclear to me under what circumstances this method would ever return an undefined value and what the motivation for this change was. Was this change in type signature a mistake that can be reverted? Or can we explain in the JSDoc and changelog when an undefined should be returned?

[Conduitry]

The built-in default decode won't return undefined because it's basically just decodeURIComponent. The question I have, then, is why a user's custom decode should be allowed to return undefined and have that come out in the object returned by parse(). What does this achieve?

[blakeembrey]

@benmccann This is a function passed in to decode, same as before. It could have returned any before 1.0, so there hasn't been a change here that should affect users.

it means we would potentially be passing undefined values to our users where we weren't before

Any missing keys are undefined.

what circumstances this method would ever return an undefined value

This method doesn't return undefined, parse accepts an option that may return undefined instead of always requiring a string to be returned. The user-facing behavior of changing decode away from the default to one that returns undefined would be that it appears to be missing keys.

As @Conduitry mentions, the default function used for decode hasn't changed, so it still doesn't return undefined. There isn't a breaking change with this, it just clarifies in types that only string | undefined should be returned instead of any before.

why a user's custom decode should be allowed to return undefined and have that come out in the object returned by parse()

It allows someone to override the behavior of parsing to skip invalid cookies. For example, decodeURIComponent will error if an invalid percent encoding exists, and it may be useful to skip over that value (but the default keeps the invalid string).

Ref:

cookie/src/index.ts

Lines 128 to 134 in e739f41

	if (obj[key] === undefined) {
	let valStartIdx = startIndex(str, eqIdx + 1, endIdx);
	let valEndIdx = endIndex(str, endIdx, valStartIdx);
	const value = dec(str.slice(valStartIdx, valEndIdx));
	obj[key] = value;
	}

It is worth noting that there are actual breaking changes with moving from pre-1.0 to 1.0 mentioned in https://github.com/jshttp/cookie/releases/tag/v1.0.0 (e.g. the removed try..catch and changed quote parsing).

[blakeembrey]

Worth linking to what @Conduitry mentions, the default decode behavior hasn't changed.

Version 1:

cookie/src/index.ts

Lines 362 to 370 in e739f41

	function decode(str: string): string {
	if (str.indexOf("%") === -1) return str;
	try {
	return decodeURIComponent(str);
	} catch (e) {
	return str;
	}
	}

Version 0:

cookie/index.js

Lines 304 to 308 in d19eaa1

	function decode (str) {
	return str.indexOf('%') !== -1
	? decodeURIComponent(str)
	: str
	}

combined with

cookie/index.js

Lines 329 to 335 in d19eaa1

	function tryDecode(str, decode) {
	try {
	return decode(str);
	} catch (e) {
	return str;
	}
	}