graphql type signature doesn't match its result

[bobvanderlinden]

The type signature of the graphql call says the result is a GraphQlQueryResponse, which is defined as:

graphql.js/src/types.ts

Lines 33 to 48 in c1fa150

	export type GraphQlQueryResponse = {
	data: { [key: string]: any } | null;
	errors?: [
	{
	message: string;
	path: [string];
	extensions: { [key: string]: any };
	locations: [
	{
	line: number;
	column: number;
	}
	];
	}
	];
	};

A object that has a data attribute and an optional errors attribute.

However, the implementation of the graphql call returns something entirely different:

graphql.js/src/graphql.ts

Lines 41 to 49 in c1fa150

	return request(requestOptions).then(response => {
	if (response.data.errors) {
	throw new GraphqlError(requestOptions, {
	data: response.data as Required<GraphQlQueryResponse>
	});
	}
	return response.data.data;
	});

It returns the contents of the data attribute and passes the errors by throwing an exception.

This behaviour is very confusing, as the type information is lying about the implementation. This resulted in some downtime on my application after an update of probot, as I presumed in my tests and mocks that the type information is correct. Refactorings were made to accomodate the new type signature of the graphql call, however the graphql call is the part that is mocked in tests.

Since this is something that I think more people would run into, I'm a bit doubtful the above findings are indeed correct. This problem did not seem to be reported yet? Am I doing something wrong?

I thought the implementation would need to look something like:

return request(requestOptions).then(response => response.data)

[wolfy1339]

Thanks for the well written report!

That code has been written like that since before the migration to TypeScript.

If I would change the return value to Promise<GraphQlQueryTesponse['data']> would that fix this issue?

[bobvanderlinden]

I personally would just change the type of GraphQlQueryTesponse to be any (or whatever type GraphQlQueryTesponse['data'] has currently). This keeps backwards compatibility for Javascript applications.

Ideally I'd like to see the implementation match the current type information in the sense that it will not throw an exception, but just return { data, errors }. The exception is not documented and it's very unclear how to get the actual errors and data from the GraphqlError object. The fields are named data and errors, but this is not documented nor does the type information show this afaik.

Returning { data, errors } also matches what you'd expect from a GraphQL call. Even if there are errors, the data field can still contain almost all information you were requesting. The whole idea of GraphQL is to fetch a bunch of information in one go. If parts of that information was not able to be retrieved does not mean that the whole call failed (which the error currently implies).

[bobvanderlinden]

Just to give an impression how I'm now handling the graphql call:

https://github.com/bobvanderlinden/probot-auto-merge/blob/b1412b5b66f2ff590bb5df87732a994ba0dff263/src/pull-request-query.ts#L36-L61

[wolfy1339]

Thanks, for the explanation, I will try and look into it

[gr2m]

Thanks @wolfy1339.

The types are wrong, graphql() resolves with the value of data, not with { data, errors }, because if errors are present, graphql() rejects instead.

Sorry for the trouble :(

[octokitbot]

🎉 This issue has been resolved in version 4.0.1 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀