[WIP] Add 'sortIntrospectionQuery' utility function#1185
[WIP] Add 'sortIntrospectionQuery' utility function#1185IvanGoncharov wants to merge 1 commit intographql:masterfrom
Conversation
| dummy: String | ||
| } | ||
|
|
||
| type Query implements FooA, FooB, FooC { |
There was a problem hiding this comment.
Should be type Query implements FooA & FooB & FooC fixed #1179
mjmahone
left a comment
mjmahone
left a comment
There was a problem hiding this comment.
I'm not sure how much we should value simplicity (take in $ReadOnlyArray, output $ReadOnlyArray) and DRYness vs copies with slightly more complexity. I don't think copies are very costly in this case, but I haven't benchmarked this recently so I don't really know. So all the suggestions below are just suggestions, as I think they'd make the code a little less clear in the process.
| return { | ||
| name: directive.name, | ||
| description: directive.description, | ||
| locations: sortBy(directive.locations, x => x), |
There was a problem hiding this comment.
there's no reason to use sortBy here: just directive.locations.slice().sort((a, b) => a.compareLocale(b));
This is useful because then we can make sortBy take a mutable array, and avoid one array-copy-per-sortByName
| if (!Array.isArray(array)) { | ||
| throw new Error('Expected array got: ' + (array: empty)); | ||
| } | ||
| return array.slice().sort((obj1, obj2) => { |
There was a problem hiding this comment.
a bit of a nit, but we can remove this slice with a bit of trickery. We also will no longer need the special sortBy<T>.
function sortFields(
fields: $ReadOnlyArray<IntrospectionField>,
): $ReadOnlyArray<IntrospectionField> {
return sortByName(fields.map(field => ({
name: field.name,
description: field.description,
args: sortByName(field.args.slice()),
type: field.type,
isDeprecated: field.isDeprecated,
deprecationReason: field.deprecationReason,
}));
}
// This cannot be called on an original ReadOnly array, or it would mutate them
function sortByName<T: { +name: string }>(
array: Array<T>,
): $ReadOnlyArray<T> {
return array.sort((obj1, obj2) => obj1.name.localeCompare(obj2.name));
}
This ends up being a little more code (you now have to slice() whenever you want to sortByName), but now we save one array copy per field list.
| queryType: schema.queryType, | ||
| mutationType: schema.mutationType, | ||
| subscriptionType: schema.subscriptionType, | ||
| types: sortByName(schema.types).map(sortType), |
There was a problem hiding this comment.
similar to below, given sortByName now takes in a mutable array, we could save one copy here vs. the current code if we flipped this:
types: sortByName(schema.types.map(sortType)),
| mutationType: schema.mutationType, | ||
| subscriptionType: schema.subscriptionType, | ||
| types: sortByName(schema.types).map(sortType), | ||
| directives: sortByName(schema.directives).map(sortDirective), |
IvanGoncharov
force-pushed
the
sortIntrospectionSchema
branch
from
1764f8e to
b3da32d
Compare
IvanGoncharov
force-pushed
the
sortIntrospectionSchema
branch
from
b3da32d to
032b615
Compare
@mjmahone Great suggestion 👍 What do you think? I also remembered that |
IvanGoncharov
changed the title
|
I changed my mind sorting introspection have one big disadvantage you lose a lot of data when you convert schema to introspection (resolve callback, directives, etc.). I started working on Please don't merge this PR |
|
Going to close this PR at your suggestion, @IvanGoncharov I agree with you that the right place for sorting is the GraphQLSchema instance itself - introspection is an intermediate representation, and since we have utilities for converting to and from introspection and schema instances, support for sorting a schema instance can be easily derived to sort any other intermediate representation, including introspection |
4 participants
Based on discussion in #941 and #889
Some of GraphQL servers doesn't preserve the order of types/fields and we need to sort them to provide consistent results. I think it is the primary use case for schema sorting so it makes sense to sort the result of introspection query and it makes implementation super simple.
As for other use-cases (e.g. tests) you can query introspection with
graphqlSyncthen sort it and build sorted schema usingbuildClientSchema.