add lexicographical sorting rules for schema stitching support

[dustinsgoodman]

Background

I've been waiting for https://github.com/cjoudrey/graphql-schema-linter/pull/240 to get merged which it did recently and I'm super excited about it! However, when I went in to remove all my custom rule hacks to use the default ignore list instead, I found that the type alphabetical rule was still failing for me.

My project uses schema stitching so we're using statements like extends type Query { ... } in it. By default, when GraphQL generates the schema, it merges those fields as follows:

// inputs
type Query {
  a: A
  c: C
}

extends type Query {
  b: B
  d: D
}

// output
type Query {
  a: A
  c: C
  b: B
  d: D
}

For this rule to work, I need this to be sorted. graphql-js has provided a function lexicographicSortSchema to enable stitched schemas to be sorted at runtime. This now outputs the desired outcome of:

type Query {
  a: A
  b: B
  c: C
  d: D
}

Issue

This libraries alphabetical sorting uses JS's sort function out of the box which is different than lexicographical sorting provided by the graphql toolkit. This creates a problem where the following fails the lint rule:

type Query {
  user(userId: ID!): User
  users: [Users]
  userTemplate: UserTemplate
}

This happens because of the differences in alphabetical and lexicographical sorting as follows:

// alphabetical
> ['users', 'user', 'userTemplate'].sort()
["user", "userTemplate", "users"]
// lexicographical
>  ['users', 'user', 'userTemplate'].sort((a, b) => a.localeCompare(b))
["user", "users", "userTemplate"]

Proposed Solution

Rather than introducing a breaking change to the alphabetical rules which people are probably using, let's introduce a new set of rules for lexicographical sorting that people can opt into if they're running into the same issues with schema stitching and lexicographicSortSchema.

[dustinsgoodman]

Resolves https://github.com/cjoudrey/graphql-schema-linter/pull/255

[dustinsgoodman]

@cjoudrey whenever you get a chance, I think this would be a nice QoL improvement for folks using schema stitching. :smile:

[cjoudrey]

@cjoudrey whenever you get a chance, I think this would be a nice QoL improvement for folks using schema stitching. 😄

Hey @dustinsgoodman,

Sorry for the radio silence. I've been going back and forth with this in my head.

Thanks for opening this pull request. I appreciate the effort you put into it adding tests and modifying the README. Really appreciate it! ❤️

While I think it's great that you added this as new rules in order to preserve the behaviour of the existing rules, I wonder if this will be problematic in a different way. People will effectively need to opt out of one of the two sets of rules because they conflict with one another.

The library doesn't support this at the moment, but I wonder if perhaps this could be a good use case for adding options to rules.

Something like this:

{
  "rules": ["enum-values-sorted-alphabetically"],
  "schemaPaths": ["path/to/my/schema/files/**.graphql"],
  "customRulePaths": ["path/to/my/custom/rules/*.js"],

  # new configuration to support this:
  "rulesOptions": {
    "enum-values-sorted-alphabetically": { "lexicographical": true },
  }
}

It'll require a bit more effort and thinking, but I think this might be a better way to gracefully roll this out.

What do you think?

[dustinsgoodman]

Sorry for the radio silence. I've been going back and forth with this in my head.

No worries! I appreciate the response 😄

Thanks for opening this pull request. I appreciate the effort you put into it adding tests and modifying the README. Really appreciate it! ❤️

No problemo! I have the fix running locally as a custom rule but I wish it was part of this library instead. Was easy to share and looks like others wanted it too.

While I think it's great that you added this as new rules in order to preserve the behaviour of the existing rules, I wonder if this will be problematic in a different way. People will effectively need to opt out of one of the two sets of rules because they conflict with one another.

The library doesn't support this at the moment, but I wonder if perhaps this could be a good use case for adding options to rules.

That is definitely a good concern. My only counter point is that all the rules are opt in right now anyway, so I feel like people will be reading the docs to figure out which rules to include anyway. There could be notes about alphabetical and lexicographical not being compatible. That being said...

Something like this:

{
  "rules": ["enum-values-sorted-alphabetically"],
  "schemaPaths": ["path/to/my/schema/files/**.graphql"],
  "customRulePaths": ["path/to/my/custom/rules/*.js"],

  # new configuration to support this:
  "rulesOptions": {
    "enum-values-sorted-alphabetically": { "lexicographical": true },
  }
}

It'll require a bit more effort and thinking, but I think this might be a better way to gracefully roll this out.

What do you think?

I do like this conceptually and it really opens the door for other linting configurables for the other rules and future rules. My only thought for the rule config is it should maybe the following instead:

"rulesOptions": {
  "enum-values-sorted-alphabetically": { "sortOrder": "lexicographical" }
}

I know for sure alphabetical and lexicographical would be good options for the sorting, but there might be other sorting options like this user was maybe alluding to in #255. I was basing the lexicographical decision on the util function that the graphql library provides for schema sorting but I wonder if other non-English codebases might benefit for this instead. I can whip up a demo PR quickly this weekend. My main 2 requirements for this change were (1) it fixes the sorting problem and (2) it doesn't introduce a breaking change which both ideas accomplish. I think yours will be more intuitive and DRY though. 😄

[cjoudrey]

I do like this conceptually and it really opens the door for other linting configurables for the other rules and future rules. My only thought for the rule config is it should maybe the following instead:

"rulesOptions": {
  "enum-values-sorted-alphabetically": { "sortOrder": "lexicographical" }
}

I like that! 😄 Seems better than what I originally suggested.

[dustinsgoodman]

@cjoudrey Ok, I think we're updated with all the desired changes so it runs from config now instead of by a new rule. Check it out and let me know what you think!

[dustinsgoodman]

@cjoudrey Just pinging you again in case you missed the update. 😄

[cjoudrey]

@cjoudrey Just pinging you again in case you missed the update. 😄

Hey Dustin, sorry for the delay. I'll get to this shortly. Thanks a lot for the contribution. ❤️

[dustinsgoodman]

Hey @cjoudrey! Hope you're doing well. Just guessing you've been busy with other work and that's ok. That being said, just checking in about this one. Got a couple of projects waiting on this to fix some issues. Let me know if there's anything I can do to help.

[cjoudrey]

Hey Dustin, thanks for your patience again. It's been a busy last two months. 😅

I'll get this released shortly.

[dustinsgoodman]

@cjoudrey No worries and thank you so much for taking the time! I hope things slow down for you and you have a happy holiday season. :heart:

[cjoudrey]

Thanks! ❤️ Wishing you the same.