Feat/#873 retrieve remote schemas

[ciaranschutte]

• retrieval of remote schemas
• responses build to full schema using gql tools (means we can use the well tested inbuilt methods without manually parsing a gql request as intermediary objects)
• output is array of full schemas as GraphQlObjects

Please note this is part of the bigger schema federation work merging into feat/featured_search
The broken build issues are for some missing TS types, implicit anys
These will be addressed before merging full feature into develop branch

[joneubank]

Should not use this type assertion. Promise.allSettled returns a PromiseSettledResult<T> where T is your type.

This is important to use as any rejected response will NOT have a .value property, we need to always check for the .status in each element of network queries array.

See type information for PromiseSettledResult<T>: https://github.com/microsoft/TypeScript/blob/main/src/lib/es2020.promise.d.ts#L11

[joneubank]

.value won't exist on rejected promises. We can filter those out if we only care about succeses:

const schemaResults = networkQueries
		.filter(
			(result): result is PromiseFulfilledResult<FetchRemoteSchemaResult> =>
				result.status === 'fulfilled',
		)
		.map((networkResult) => {
			const { config, introspectionResult } = networkResult.value;

[ciaranschutte]

that's how the code is currently handling

[ciaranschutte]

@joneubank addressed feedback. TS building. please take another look when you have some time.

[joneubank]

Looking strong throughout.

The axios request failure handling I think is not what you would expect to see so consider that comment and if there are changes needed.

[joneubank]

🙏

[joneubank]

Looking at how this is used, we don't really care about the specific contents of the variables. This function is doing no work to ensure that the variables expected in the query are included in variables (or vice versa). The only thing we know is we want to be able to run JSON.stringify(variables) which takes any. In practice, we expect this parameter to be an object with some variable values which themselves could be anything, and we probably don't want it to be string or number etc. So in this case I might reccomend:

variables: Record<string, any>

[joneubank]

Noticed this function isn't actually used in your code here, is that expected?

[ciaranschutte]

expected. #877 (comment)

[joneubank]

TSDoc will be wanted before feature branch is complete.

[joneubank]

I think your TSDoc is incorrect, instead of returning an axios instance that you can use to send future requests, this sends your defined request with axios and returns the AxiosResponse (or throws error, stupid axios).

[justincorrigible]

Great feedback, useful insight... but let's try keeping unhelpful personal opinions to a minimum.

[joneubank]

why not just:

introspectionResult?: IntrospectionQuery

[joneubank]

👍

[joneubank]

Axios throws errors on status 4xx and 5xx. That means that the else block you have above is behaviour that you are likely expecting to be handled here as well. You should try checking if the error here is an instance of AxiosError, and if so you can handle these error statuses in the catch block. I imagine you will want to throw your NetworkAggregationError.

[joneubank]

With TS 5.5 we no longer require the result is return type :D

[joneubank]

PS. don't need to do introsepectionResult: undefined if you have the type defined as introspectionResult?: IntrospectionQuery

[joneubank]

I think your TSDoc is incorrect, instead of returning an axios instance that you can use to send future requests, this sends your defined request with axios and returns the AxiosResponse (or throws error, stupid axios).

[joneubank]

The comment here doesn't tell me anything new. You can use TSDoc on types as well to leave a description of the type in intellisense for future devs that use it.

[joneubank]

This is a helpful comment but it wasn't easily understandable. I had to read the type information to understand what you mean. Maybe something like:

Suggested change

	// additional properties after initialising network
	export type NetworkAggregationConfig = NetworkAggregationConfigInput & {
	schema?: GraphQLSchema;
	};
	/**
	* Complete aggregation config that defines the network search setup. This includes the original
	* config information plus computed fields that are generated in the network search initialization process.
	*/
	export type NetworkAggregationConfig = NetworkAggregationConfigInput & {
	schema?: GraphQLSchema;
	};

[justincorrigible]

That's a nice improvement, for sure!
Naive question: am I the only one who gets bugged by the line length inconsistency?
personally, I'd chop it like this

* Complete aggregation config that defines the network search setup.
* This includes the original config information plus computed fields
* that are generated in the network search initialization process.

[justincorrigible]

wouldn't typeof input === 'object' inherently cover the !!?
(an object cannot be falsy)

Suggested change

	!!input && typeof input === 'object' && '__schema' in input && typeof input.__schema === 'object';
	typeof input === 'object' && '__schema' in input && typeof input.__schema === 'object';

[justincorrigible]

does it matter that this function would allow input to be an array customised to include __schema?

[ciaranschutte]

1st point:
typeof null is object - <3 js (so '__schema' in input would break)

[ciaranschutte]

2nd point:
I don't think that's a concern. I'm using the getIntrospectionQuery which is part of the graphql library (conforming to spec) and this check is only happening to check response data from a GQL server.

It queries an automatically generated field in a graphql server, not a user created field.
For an array to occur the graphql spec would need to be changed.

[justincorrigible]

is there a null.__schema? otherwise, !! is still redundant

[joneubank]

It would be nice if these messages were more descriptive. The best error messages let the reader know what was being attempted, why it failed, and (if possible) what action should be taken.

It is likely that this is about on par with other error messaging in this repo, but working to improve these as we go would be a huge help.

[ciaranschutte]

for sure - this is just the "retrieve schema" step
buildClientSchema is not implemented
will be keeping this in mind working on the "build schema" ticket