Feature request: Guillotine should take `allowContentType` into account

[tom]

Hi,

Problem

The GraphQL-types for ConstentSelector and ImageSelector is Content. Which is a very wide type that encompasses all content-types in the system, when in reality the content can only be of one or two types.

This is especially unelegant when the data in reality only can be of one type, and we have to use an inline fragment to query the data, instead of just doing it directly.

Problem 2

I just shared a post about how to Generate frontend TypeScript-types from GraphQL Schema.

It is a very useful technique, but it would be even better if the Schemas would be a bit more precise.

My big problem is that GraphQL-queries generate MegaBytes of unnecessary TypeScript-types that are hard to work with.

That is why I’m “hacking” this myself, by overwriting the GraphQL-types of ContentSelectors and ImageSelectors with more presise types, like this:

import { 
  createWebSocketData, 
  initWebSockets, 
  createSchema, 
  type Context
} from "/lib/guillotine";
import {
  reference,
  list,
  execute as executeGraphQL,
  type GraphQLResolverEnvironment,
  type CreateObjectTypeParams,
} from "/lib/graphql";

...

const schema = createSchema({
  applications: ["com.nerdforge"],
  creationCallbacks: {
    // ARTICLE DATA
    com_nerdforge_Data: (context: Context, params: CreateObjectTypeParams) => {
      // Authors is a ContentSelector, with two "allowContentTypes"
      params.fields.authors.type = list(
        context.schemaGenerator.createUnionType({
          name: context.uniqueName("com_nerdforge_Article_Data_Authors"),
          types: [
            reference("com_nerdforge_Employee"), 
            reference("com_nerdforge_ExternalPerson")
          ],
          typeResolver: (content: Content) => context.contentTypeMap[content.type] ?? context.types.untypedContentType,
        })
      );

      // ImageSelector always has type "media_Image"
      params.fields.image.type = reference("media_Image");
    },

I wish that Guillotine would do this automatically for me!

Solution for ImageSelector

So I suggeset changing the type for ImageSelector in Guillotine to reference("media_Image") instead of reference("Content").

Solution for ContentSelector

Take allowContentType into account, and set the GraphQL type of that field to be either the one allowed content type, or a union of the allowed content types.

Thanks,
– Tom

[tom]

Related problem: nullability

A related problem is that a lot of fields from Guillotine that in reality is not nullable (e.g displayName), is not set to nonNull() in Guillotine.

Similarly fields in content types where <occurrences minimum="1"/> should result in a GraphQL-type that is not nullable.

The problem for me is when I run TypeScript-code generation like described here, I get types back that are much more difficult to work with, then if they were not nullable.

---

Example

The “null | undefined” type is in reality not true for displayName, so we are making this a lot more difficult to program against then it should be.

export type ContentByPathQuery = {
  __typename?: 'Query'
  guillotine?:
    | {
        __typename?: 'HeadlessCms'
        get?:
          | { __typename?: 'base_Folder'; displayName?: string | null | undefined }
          | { __typename?: 'base_Media'; displayName?: string | null | undefined }
          | { __typename?: 'base_Shortcut'; displayName?: string | null | undefined }
          | { __typename?: 'base_Structured'; displayName?: string | null | undefined }
          | { __typename?: 'base_Unstructured'; displayName?: string | null | undefined }
          | { __typename?: 'media_Archive'; displayName?: string | null | undefined }
          | { __typename?: 'media_Audio'; displayName?: string | null | undefined }
          | { __typename?: 'media_Code'; displayName?: string | null | undefined }
          | { __typename?: 'media_Data'; displayName?: string | null | undefined }
          | { __typename?: 'media_Document'; displayName?: string | null | undefined }
          | { __typename?: 'media_Executable'; displayName?: string | null | undefined }
          | { __typename?: 'media_Image'; displayName?: string | null | undefined }
          | { __typename?: 'media_Presentation'; displayName?: string | null | undefined }
          | { __typename?: 'media_Spreadsheet'; displayName?: string | null | undefined }
          | { __typename?: 'media_Text'; displayName?: string | null | undefined }
          | { __typename?: 'media_Unknown'; displayName?: string | null | undefined }
          | { __typename?: 'media_Vector'; displayName?: string | null | undefined }
          | { __typename?: 'media_Video'; displayName?: string | null | undefined }
          | {
              __typename?: 'com_nerdforge_Article'
              displayName?: string | null | undefined
              data?:
                | {
                    __typename?: 'com_nerdforge_Article_Data'
                    intro?:
                      | { __typename?: 'RichText'; processedHtml?: string | null | undefined }
                      | null
                      | undefined
                  }
                | null
                | undefined
            }
          | { __typename?: 'portal_Fragment'; displayName?: string | null | undefined }
          | { __typename?: 'portal_PageTemplate'; displayName?: string | null | undefined }
          | { __typename?: 'portal_Site'; displayName?: string | null | undefined }
          | { __typename?: 'portal_TemplateFolder'; displayName?: string | null | undefined }
          | null
          | undefined
      }
    | null
    | undefined
}

[tsi]

Hi Tom.
This approach would potentially break your API, simply by changing “allow-content-type” for a single input. Also, it would mean we would have to generate new types for every single field, groing the schema fast.
It is very important to provide stable GraphQL API’s, especially when they are generated like we do in Guillotine.

Also, for ImageSelector, we currently return multiple content types, both media:image and media:vector (svg’s).

What are your comments to this?

[tsi]

Hi Tom - can you post a separate topic for nullabillity?

[tom]

You can find it here:

Feature request: Guilliotine should take nullability into account

This problem was first posted as a comment in another feature request. Nullability A lot of fields from Guillotine that in reality is not nullable (e.g displayName), does not use nonNull() in Guillotine, resulting in more complex types then necessary. Similarly fields in content types where <occurrences minimum="1"/> should result in a GraphQL-type that is not nullable. The problem for me is when I run TypeScript-code generation like described here, I get types back th…

[tom]

Hi Thomas

Thanks for the feedback. Here are my replies:

@tsi

This approach would potentially break your API, simply by changing “allow-content-type” for a single input.

You are not really breaking the API are you? But you can potentially be breaking GraphQL-queries that are using that API when you remove an allowed content type.

But is that so bad? It would only force the developer to remove dead code from their queries.

@tsi

it would mean we would have to generate new types for every single field, groing the schema fast.

Single allowContentType

The best case scenario here is when there is only one allowContentType, which means that the type is a reference to that type (so no new types is needed).

In my code, this is one of the most common ways allowContentType is used. I reference related content of a specific type.

And it would be really nice to not always need the inline fragment in the Graphql-query. It’s much easier for beginners too.

Multiple allowContentType

For cases where more then one allowContentType is used, Guillotine need to create a union of those types.

For most projects I don’t think this will happen a lot of times. E.g for SSB I could only count one case of this pattern being used.

So it wouldn’t create an explosion of new union-types, normally only a few.

One could also imagine Guillotine creating a naming scheme for Unions making them reusable.

Image selector

@tsi

for ImageSelector, we currently return multiple content types, both media:image and media:vector

It sounds like it would be pretty easy to create a union of media:image and media:vector, and if they get a common interface – of boy – it would be easy to query against in GraphQL.

Closing thoughts

Working a lot with types in TypeScript has tought me that creaing types as narrow as possible eliminates potential bugs, and makes it easier for the developer programming against those interfaces.

I think this is true for GraphQL and Guillotine too. The GraphQL-types should be as truthfully as possible describing the data it exposes, without adding many impossible side cases.

[tom]

I also want to share this video (which I think has a good explanation of why corner cases are bad):

Why are corner cases the devil? 😈

[tsi]

Hi Tom.
Forcing a client to update is effectively the same as breaking the API. Remember that the Guillotine API may be used by multiple concurrent clients, and will update instantly when you deploy changes to your model.

Based on the fact that we aim to provide an API that is as stable as possible (even if it is generated), we will not change this behavior ATM. What is more likely is that we may provide more flags and annotation capabilities such as “deprecated” etc for schema inputs, that may provide additional flexibility.