New Rule: Array bang bang

[goldcaddy77]

Many have argued that the default behavior of Arrays in the schema should be [Obj!]!: ex:

type Foo {
  bars: [Baz!]!
}

Most of the time, when you see:

type Foo {
  bars: [Baz]
}

It's a mistake, because then null and [null, null] are valid values - which is most likely not the cast. Thoughts on a new rule for this?

[cjoudrey]

That's an interesting thought @goldcaddy77.

I've definitely seen cases where someone wanted [Baz!] but used [Baz] by mistake.

That said, I don't feel comfortable enforcing [Obj!] or [Obj!]! in the default list of rules because I'm sure some people have legitimate cases for this.

One idea could be to expose this rule in another package and I wouldn't mind linking to it in the README.md. People could load it as a custom rule when executing graphql-schema-linter.

[goldcaddy77]

This is actually a hard rule at my company. Thoughts on exposing it and just not having it in the default rule set? So if we agree [Obj] is bad, that leaves us with:

• [Obj!] - this allows null as a valid value that can be passed back through the API
• [Obj]! - this requires an array, but [null] is a valid value

I understand there is probably some weird edge case where one of these needs to be supported, but in 95%+ of scenarios, people should be using [Obj!]!.

Can anybody think of a legitimate reason where [Obj], [Obj!], [Obj]! would be a best practice?

[n1ru4l]

@goldcaddy77

type Comment {
  id: ID!
  text: String
}

type Post {
  id: ID!
  comments: [Comment!]
}

type Query {
  post(postId: ID!): Post
}

type Mutation {}

Lets say loading the comments of Post failed, by using [Comment!] you can throw an Error in the resolver for your post comments and that will be part of the response. The property comments on the Post will be null. But you still have the partial data for the post.

{
  "data": {
    "post": {
       "id": "1",
       "comments": null,
    }
  },
  "errors": [
     { "something": "...." }
  ]
}

However if you use [Comment!]! and throw an Error in your post comments resolver the post will ne null since it cannot exist without the comments field being an Array.

{
  "data": {
    "post": null
  },
  "errors": [
     { "something": "...." }
  ]
}

You could fix this by returning an empty Array and ignoring the error. However in that case the client that consumes your graphql endpoint will never know there might be some comments and e.g. you cannot hande the UI for retry etc.

I hope this clarifies different use cases

[goldcaddy77]

I'd argue that you always want to return an empty array for comments. You can still return the partial data with an error, you'd just have "comments": [] The post either has comments or it doesn't. If it has comments, you could get [Comment1, Comment2], if it doesn't, you should get []. Without being this strict, your clients will always need to handle the different response structures client-side.

Which API would you rather work with?

<div class="table">
  {post.comments.map((comment: Comment, index: number) => this.renderComment(rowData, index))}
</div>

...or...

<div class="table">
  {post.comments !== null && post.comments.map((comment: Comment, index: number) => this.renderComment(rowData, index))}
</div>

You'll need to do this sort of check everywhere you use comments instead of being able to trust that you'll get an array ^^^

[goldcaddy77]

@cjoudrey - do you know of any external rules packages that exist I could use as a template? I'm down to write the bang bang rule.

[n1ru4l]

@goldcaddy77 It depends on the use-case and preference but I personally try to reflect any occurred error in the UI.

[cjoudrey]

@cjoudrey - do you know of any external rules packages that exist I could use as a template?

@bwillis might have some experience with this, but unfortunately I don't.

My naive approach would be to create a new NPM package and include a src/rules/enforced_list_nullability.js (or name it how you wish) in it. I would then add graphql-schema-linter and this new package to my project's package.json. You can then use the --custom-rule-paths <paths> to include the rules from the package. This isn't great, but it'll work.

In an ideal world, there could be some sort of plugin system like what ESLint provides so people don't need to mess with paths. I haven't really thought about this a ton. When --custom-rule-paths <paths> got added, I mostly wanted to provide a way for people to add custom rules.

If you don't need to share this rule across multiple projects you could also just add the file to your existing project and include it using --custom-rule-paths <paths>.

In terms of how to write a custom rule, there are some notes at the end of the README. I mostly just leveraged existing work from GraphQL.js.

Let me know if you have other questions. 😄

[bwillis]

@bwillis might have some experience with this, but unfortunately I don't.

Haven't done it as a separate package, we include our dependencies directly in our project something like this:

// .graphql-schema-linterrc
{
  "customRulePaths": [
    "./path/to/package/*.js"
  ],
  "rules": [
    "custom-rule-1",
    "custom-rule-2",
    "enum-values-sorted-alphabetically",
    "deprecations-have-a-reason",
    "types-are-capitalized",
    "relay-connection-types-spec",
  ]
}

This should highlight the path issue @cjoudrey is talking about. @goldcaddy77 if you do find a solution please share or want to talk in more depth about approaches, maybe a new issue would be in order.

[Caerbannog]

This package by @morris provides a rule that checks for null list items: https://www.npmjs.com/package/graphql-schema-linter-extras It does not check for null lists. Here is the code. ISC license according to package.json.

const {
  ValidationError
} = require("graphql-schema-linter/lib/validation_error");

module.exports = { ListItemsNotNull };

function ListItemsNotNull(context) {
  return {
    ListType(node, key, parent, path, ancestors) {
      if (node.type.kind !== "NonNullType") {
        const ancestorNames = ancestors
          .concat([parent])
          .filter(it => !!it.name)
          .map(it => it.name.value);

        const p = ancestorNames.slice(ancestorNames.length - 2).join(".");

        context.reportError(
          new ValidationError(
            "list-items-not-null",
            `The list field \`${p}\` should not have nullable items.`,
            [node]
          )
        );
      }
    }
  };
}