Incorrect return type for CompletedBody.getJson

[leumasme]

Implementation

mockttp/src/util/request-utils.ts

Lines 233 to 237 in b50532a

	async getJson() {
	return runAsyncOrUndefined(async () =>
	JSON.parse((await completedBody.getText())!)
	)
	},

Types (why are these even separate?)

mockttp/src/types.ts

Lines 176 to 181 in b50532a

	/**
	* The contents of the response, decoded, parsed as UTF-8 string, and
	* then parsed a JSON. The response is decoded and returned asynchronously
	* as a Promise.
	*/
	getJson(): Promise<object | undefined>;

JSON.parse can not return undefined (but can return null, which is not the same) and can return more than just objects (numbers, strings).
Probably best to just type getJson as Promise just like JSON.parse is typed as any.

[pimterry]

Types (why are these even separate?)

There's a set of separate types in types.ts that are intended to cover the most important & explicitly public parts of the API interface (i.e. the core types people can depend on) in one place. For this case there's only one implementation, but for most of the other types there that's not true.

Notably every exported type in there is re-exported directly from main.ts, so they're all listed un-namespaced on the root page of the API reference docs.

Probably best to just type getJson as Promise just like JSON.parse is typed as any.

I'd very very strongly prefer to avoid introducing any unnecessarily. If I were doing this again now, it'd probably be unknown (I suspect the TS team would say the same for JSON.parse itself nowadays!).

It's better than downstream users are explicit about what they expect here (including any if they want to opt in to that) rather than being unexpectedly given a footgun.

JSON.parse can not return undefined (but can return null, which is not the same) and can return more than just objects (numbers, strings).

Sure, but this method can still return undefined. It will do so if the JSON is not parseable, or if the body is not decodeable at all (e.g. corrupted gzip, unknown encoding) - that's what runAsyncOrUndefined or does. Part of the goal of this type is to make that explicit, so you have to check if (!jsonValue) to handle unparseable data. It's a poor man's Maybe (would I do it this way if I did it again now? Maybe not, but it's very simple to write and use, and it mostly works).

Anyway, yes, the object part could certainly be improved. We should similarly make it easy to use arrays, primitives & null here. The challenge is a) avoiding breaking changes in these types, so far as possible, and b) avoiding introducing any here.

The method itself should probably be generic with a default parameter too, so you can easily set an explicit type if you want to.

microsoft/TypeScript#1897 has a lot of similar related discussion. I think in this case, we could keep it simpler & stricter: rather than trying to come up with a fully flexible type we'd just provide a convenient base type, and encourage asserting to the expected type directly from there instead. For example that could be just {} | [] | string | number | boolean | null.

That could create some breakage, but I think only in some very specific cases like if (bodyJson && 'x' in bodyJson). I don't love that, but it is more correct and I think it's unavoidable if allowing primitives here... I suspect most users are casting fairly directly anyway so it shouldn't matter.

All put together, that would give us something like:

getJson<R = {} | [] | string | number | boolean | null>: Promise<R | undefined>

Would that work for you?

If so, a PR to add this and some example tests to cover a few of the relevant cases would be welcome, feel free!

[leumasme]

If I were doing this again now, it'd probably be unknown (I suspect the TS team would say the same for JSON.parse itself nowadays!).

Good point (and likely true considering their partial move from any to unknown on caught values in a trycatch).

that's what runAsyncOrUndefined or does

Oops, sorry - looks like I didn't look at this properly

For example that could be just {} | [] | string | number | boolean | null.

I don't think this does quite what you think it does. [] is an array specifically with 0 elements, and {} as a type behaves very weirdly, ex:

let x: {} = 1;

is valid (Video Explainer)

I don't believe any of the issues mentioned in the original issue post you linked apply for this usecase since the inferred return type of the function is Promise<any> anyway and the type is probably not expected to be used much more than that, but I'm not sure.

If typed as unknown, it should likely be added to the doc comment that undefined will be returned in case of a parse error for any reason - well this should probably done either way.

I generally like to contribute, but I don't feel too confident adding tests to a project that im not that familiar with ^^'

[pimterry]

I generally like to contribute, but I don't feel too confident adding tests to a project that im not that familiar with ^^'

Ok, fair enough! Personally this doesn't seem like a huge problem and I'm super busy right now, so it's unlikely I'm going to look into this in more depth in the short term myself. I'll keep an eye on it for future in case it is causing big problems for more users, or do feel free to open a PR in the meantime if this is important to you.