Add design guide for graphql naming conventions

[darrikonn]

https://github.com/island-is/handbook/blob/graphql-naming-conventions/docs/api-design-guide/graphql-naming-conventions.md

[eirikurn]

Great!

I prefer namespace prefixing. It is closer to this proposal, which is being discussed and considered (tldr, they are weighing implementation complexity vs letting the community handle namespacing on their own).

Type:

1. search_Article
2. Search_Article
3. Article_Search

Top level field (I guess nested fields don't need namespacing unless they refer to types from another namespace):

1. search_article
2. Search_article
3. article_Search

Mutation:

1. search_indexArticle
2. Search_indexArticle
3. indexArticle_search

My preference is #1 because:

1. Namespaces are usually prefixed in environments that don't support them natively. Also more common in graphql schemas in the wild afaict.
2. All of our (nest/nx/npm) modules are lower case.

We'll also have to consider versioning at some point, that could follow a similar namespacing rule, e.g. search_v2_Article?

[eirikurn]

We'll probably add more sections to this as time goes by, but I'd like to document one important best practice for future compatibility, to never resolve arrays directly. Always wrap arrays in some kind of payload object.

[darrikonn]

Yeah agree with the prefix after seeing the article.
But what about underscore vs. slash?

Can you elaborate on:

to never resolve arrays directly. Always wrap arrays in some kind of payload object.

[eirikurn]

I'd prefer a slash, but it's not a supported symbol in identifiers, which is why they're proposing it for the standard.

The only non-alphanumerical symbol is "_". :(

Regarding arrays, it's mainly about pagination. It's classic to start with arrays and return all items and then having no way to add cursor-based pagination with backwards compatibility.

Worth having a naming convention for pagination?

[eirikurn]

I guess this guide should be implementation independent, but practically, in NestJS, I think this involves always passing explicit names to type and resolver decorators:

@ObjectType('search_Article')
class Article {}

@Resolver()
class ArticleResolver {
  @Query(() => Article, { name: 'search_article' }
  getArticle() {}

  @Mutation({ name: 'search_indexArticle' })
  indexArticle() {}
}

We should consider creating an eslint rule to enforce this naming convention, as it is easy to forget during implementation and review.

[darrikonn]

Yeah I see!
Not keen on Worth having a naming convention for pagination?.

My vote would be relay-style pagination.

query {
  dog {
    id
    owners(first: 10, after: "TYU468HG7") {
      count
	  edges {
        cursor
        node {
          id
        }
	  }
      pageInfo {
	    hasNextPage
	    endCursor
	  }
    }
  }
}

Here we could then later decide if we wanted a cursor based pagination vs. offset based pagination.

[darrikonn]

We should consider creating an eslint rule to enforce this naming convention, as it is easy to forget during implementation and review.

Yeah, my original thought was to prefix it at runtime.

[eirikurn]

That would be ideal, but it'd be an exercise in NestJS. There is a transformSchema hook, but that happens after merging all the domains/resolvers (which either fails or overrides stuff on conflict).

We'd have to implement the code-first schema generation logic from scratch, or create a GraphQLModule in each domain and somehow merge them manually. :/

[darrikonn]

What's your thought on relay pagination style?
And your alternative (if any)?

[eirikurn]

I've never gone for it before because the pattern feels a bit bloated, with two types per connection (Connection type and Edge type), and extra wrapping/unwrapping needed both on server and client.

I would personally base our convention on the Connection pattern but simplify, skipping the Edge type, unless:

1. We want to support Relay at some point.
2. We want to follow "best practices" according to graphql.org.
3. We think there's a use case for getting per-edge cursor.
4. We feel confident in creating awesome pagination utilities on server and client (though this is useful either way).

For #1 and #2, we might also want to discuss the node pattern.

[eirikurn]

How about simplifying namespacing rules and skip the namespace symbol. Just prefix the namespace and follow casing rules? E.g.

Type: SearchArticle instead of search_Article
Field: searchArticle instead of search_article

Pros:

• Simplifies casing in queries, result objects and generated types. I.e. follows our global casing rules in generated types and data json.
• No screaming eslint rules.
• If we want, we can name server types and resolvers with the same naming conventions instead of passing name to decorators or messing with nestjs (@ObjectType() class SearchArticle {} vs @ObjectType('search_Article') class Article {}).

Cons:

• Potentially silly names (ApplicationsApplication?).
• Not obvious where namespace ends and field starts. E.g. mutations would feel prefixed by a noun (applicationsCreateApplication vs applications_createApplication).
• Potential conflict if one namespace is a prefix for another (Search-IndexResult vs SearchIndex-Result). Easy to avoid.

[darrikonn]

This answer sums it quite well why there's a per-edge cursor:

We fetch cursors on every edge so that if you have a component that asks for first:10 and have it cached and later another component asks for first:5, we can generated the correct pageInfo based on the cursors we have fetched from first:10 for first:5 without doing a server round trip.
Another case is, if some of the edges are deleted through mutations, we still have the cursors from the other edges to fetch for additional edges.

Regarding How about simplifying namespacing rules and skip the namespace symbol:
I think it makes sense for queries, like query applicationsApplication which returns ApplicationsApplication is actually super descriptive!

But IMO the mutation names would have to have a pretty specific rule. Like createApplicationsApplication is really descriptive and I'd prefer that over applicationsCreateApplication. So the general rule would be:
Name your mutations verb first. Then the module name, following the object, or “noun,” if applicable

Not sure how we'd enforce that ^?

[eirikurn]

IMHO those feel like very specific edge cases (pun intended), which is tricky to get right unless we're using Relay. Meanwhile spending more bytes and compute cycles for the general cases. But I won't die on this hill.

I think your mutation naming suggestion is fair.

Regarding enforcement, custom eslint rule to the rescue: :)

"@island-is/namespaced-graphql-schema": [
  "error",
  {
    "namespaces": [
      "applications",
      "cms",
      "search"
    ]
  }
]

It can visit graphql resolvers, types and enums based on the nestjs directives, and verify that they have one of the namespace prefixes (ignores the verb for mutations).

It can also read the valid namespace names from the repo somehow.

[darrikonn]

Ready for re-review.
Please let me know if something isn't in line with our discussion above.

[darrikonn]

Fixed based on Conversation, and due to parental leave (Congrats!) I'm gonna merge this in so it won't become stale