Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add 'sortIntrospectionQuery' utility function #1185

Closed

Conversation

IvanGoncharov
Copy link
Member

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 graphqlSync then sort it and build sorted schema using buildClientSchema.

dummy: String
}

type Query implements FooA, FooB, FooC {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be type Query implements FooA & FooB & FooC fixed #1179

Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the same thing here

@IvanGoncharov IvanGoncharov force-pushed the sortIntrospectionSchema branch from 1764f8e to b3da32d Compare December 28, 2017 20:01
@IvanGoncharov IvanGoncharov force-pushed the sortIntrospectionSchema branch from b3da32d to 032b615 Compare December 28, 2017 22:48
@IvanGoncharov
Copy link
Member Author

a bit of a nit, but we can remove this slice with a bit of trickery

@mjmahone Great suggestion 👍
I changed sortBy:

function sortBy<T>(
  array: $ReadOnlyArray<T>,
  mapToKey: T => string,
  sortValue?: T => T,
): $ReadOnlyArray<T> {
  const newArray = sortValue ? array.map(sortValue) : array.slice();
  //...
}

What do you think?

I also remembered that locations is a new field and could be missing if you work with old GraphQL API so I change sortBy to simply ignore all non-array values.

@IvanGoncharov IvanGoncharov changed the title Add 'sortIntrospectionQuery' utility function [WIP] Add 'sortIntrospectionQuery' utility function Jan 8, 2018
@IvanGoncharov
Copy link
Member Author

IvanGoncharov commented Jan 8, 2018

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 sortSchema function.

Please don't merge this PR

@leebyron
Copy link
Contributor

leebyron commented Jan 8, 2018

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

@leebyron leebyron closed this Jan 8, 2018
@IvanGoncharov IvanGoncharov deleted the sortIntrospectionSchema branch March 9, 2018 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants