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

Strongly typed unions #283

Open
jaredjj3 opened this issue Dec 13, 2021 · 6 comments
Open

Strongly typed unions #283

jaredjj3 opened this issue Dec 13, 2021 · 6 comments

Comments

@jaredjj3
Copy link

When using { onUnion } from src/types.ts, I immediately found that it was error-prone and not inherently safe against schema changes. Instead of using that, I created a strongly typed union type that wraps onUnion. I would like to share and get your thoughts on adding it as a static method on { types } from src/types.ts which will supplement { onUnion } from src/types.ts.

type DeepPartial<T> = {
  [P in keyof T]?: T[P] extends (infer U)[] ? DeepPartial<U>[] : T[P] extends object ? DeepPartial<T[P]> : T[P];
};

type Union<T extends string = string> = {
  __typename?: T;
};

type UnionSelection<U extends Union> = {
  [Typename in NonNullable<U['__typename']>]: DeepPartial<Omit<Extract<U, Union<Typename>>, '__typename'>>;
};

type Selection<S extends UnionSelection<any>, Typename extends keyof S> = S[Typename];

type Flattened<S extends UnionSelection<any>> = {
  [Typename in keyof S]: { __typename: Typename } & Selection<S, Typename>;
}[keyof S];

class types {
  // The reason why we return a function is to allow S to be inferred. Otherwise, we get the more genrealized
  // type and we have to specify all parameters, which would defeat the purpose of the convenience of not having
  // the specify a selection twice.
  static union = <U extends Union>() => <S extends UnionSelection<U>>(types: S): Flattened<S> => {
    const frags = {};
    for (const [__typename, fields] of Object.entries(types)) {
      Object.assign(frags, onUnion({ [__typename]: { __typename: t.constant(__typename), ...(fields as any) } }));
    }
    return frags as any;
  };
}

An example usage is:

type ConfirmEmailOutput = EmailConfirmation | NotFoundError | BadRequestError | ForbiddenError | UnknownError;

type EmailConfirmation = {
  __typename: 'EmailConfirmation';
  confirmedAt: string;
};

type NotFoundError = {
  __typename: 'NotFoundError';
  message: string;
};

type BadRequestError = {
  __typename: 'BadRequestError';
  message: string;
};

type ForbiddenError = {
  __typename: 'ForbiddenError';
  message: string;
};

type UnknownError = {
  __typename: 'UnknownError';
  message: string;
};

types.union<ConfirmEmailOutput>()({
  EmailConfirmation: {
    confirmedAt: types.string,
  },
  NotFoundError: {
    message: types.string,
  },
  BadRequestError: {
    message: types.string,
  },
  ForbiddenError: {
    message: types.string,
  },
  UnknownError: {
    message: types.string,
  },
});

By leveraging the __typename field, we have a discriminated union that allows union queries to be strongly typed:

image

It also requires all members of the union to be included, which prevents developers from forgetting to include a __typename from the union. In this example, I deleted the NotFoundError from the union selection object, but the compiler complains, which is the desired result. This enforces the invariant that all members of a union are required in the query:

image

What are your thoughts?

@acro5piano
Copy link
Owner

Hi, thanks for your interest.

In your case, is ...onUnion() is enough?

import { onUnion, mutation, types } from 'typed-graphqlify'

mutation('confirmEmail', {
  confirmEmail: {
    ...onUnion({
      __typename: types.constant('EmailConfirmation'),
      message: types.string,
    }),
    ...onUnion({
      __typename: types.constant('NotFoundError'),
      message: types.string,
    }),
  },
});

Details ➡️ https://github.com/acro5piano/typed-graphqlify#inline-fragment

By the way, I think it is better idea to use GraphQL's standard way to handle errors. You should return error objects defined in RFC:

@jaredjj3
Copy link
Author

In your case, is ...onUnion() is enough?

It's enough to get it to work, but I sacrifice a lot of type safety benefits:

  • Nothing enforces me to include all union members at compile time. Otherwise, I can get unexpected/unhandled types at runtime. I think this is a specific use case and the behavior should ultimately be forked into two types: types.strictUnion and types.nonStrictUnion. Alternatively, if you want a fluent API: types.union is non-strict by default and you can have is types.union.strict.

  • The resulting TypeScript type is the intersection (not union) of the included GraphQL union types. Following your example, NotFoundError should not have a confirmedAt field:

image

This means that the caller will have to know which subset of fields belong to which __typename, which partially defeats the purpose of typing out the fields. The TypeScript type of __typename is also incorrectly assigned a single literal ('NotFoundError' instead of 'EmailConfirmation' | 'NotFoundError'), so there's a chance I'd have to do some type casting downstream to make the compiler happy.


By the way, I think it is better idea to use GraphQL's standard way to handle errors. You should return error objects defined in RFC:

Thanks for the suggestion. It seems like the community is split on this. I ultimately decided to follow @leebyron's advice in graphql/graphql-spec#391 (comment):

GraphQL errors encode exceptional scenarios - like a service being down or some other internal failure. Errors which are part of the API domain should be captured within that domain.

RE: https://www.apollographql.com/docs/apollo-server/data/errors/#throwing-errors, I'm not using Apollo, so the behavior described in the link is not immediately available to me.

Originally, I've been encoding errors in the top-level errors object and extending them with an extensions.code property. This created an implicit client-server coupling that I didn't like. The clients would have to implicitly know how to map an exentsions.code to an error type, and then know what other properties are available in extensions. The only way to know during testing that an API change breaks the client is through a functional test that involves a real GraphQL server and a client.

By including error types in the domain, I can rely on the compiler to stop me from using invalid fields for a given *Error type. That's why I decided to make this request.

These articles also influenced my decision when designing the API:

@acro5piano
Copy link
Owner

Sorry my example was wrong. It should be something like this:

const m = mutation('confirmEmail', {
  confirmEmail: {
    ...onUnion({
      EmailConfirmation: {
        __typename: types.constant('EmailConfirmation'),
        message: types.string,
        confirmedAt: types.string,
      },
      NotFoundError: {
        __typename: types.constant('NotFoundError'),
        message: types.string,
      },
    }),
  },
})

And it actually becomes intersection type as you stated. However, type guard works to some extent like this:

image

I don't want to add another union helper and interface, so if you improve the current type definition instead of creating a new one, I'm happy to merge it!

It seems like the community is split on this. I ultimately decided to follow @leebyron's advice in graphql/graphql-spec#391 (comment):

Oh I didn't notice that. Thanks for sharing!

@acro5piano
Copy link
Owner

By the way, Relay also recommends your choice which uses Error type as a model. I didn't notice that again.

https://relay.dev/docs/guided-tour/rendering/error-states/#accessing-errors-in-graphql-responses

@jaredjj3
Copy link
Author

jaredjj3 commented Dec 31, 2021

Thanks for that resource. I've migrated one of my codebases to use that pattern, and having the typed errors is so far a better developer experience. The main tradeoff is that my server code is more complex, but it ultimately forced me to surface implicit behavior in tests.

I spent a few days making the union typing perfect before attempting a PR, but I was unsuccessful. I ran into an issue regarding weak types.

When specifying fields for any type of GraphQL query, it only makes sense to make the specification a deep partial. Otherwise, we lose one of the main benefits of GraphQL. This is also a hard requirement for two-way relationships. For example:

type UserOutput = User | NotFoundError

type User = {
  __typename: 'User';
  username: string;
  post: Post[];
};

type Post = {
  __typename: 'Post';
  title: string;
  body: string;
  author: User;
  reviewer: User;
};

We must be able to specify a deep partial, or there will be an infinite cycle selecting fields from User or Post.

However, using deep partials in my type definitions made the types weak. This allowed unknown keys to be specified, which was undesirable:

types.union<UserOutput>()({
  User: {
    username: types.string(),
    body: types.string(), // GOOD: the compiler does complain about this
    foobar: types.string(), // BAD: the compiler didn't complain about this
  },
  Post: {
    title: types.string(),
  },
});

I got around this by requiring the __typename to be specified, but now the API stutters, since the caller has to specify the __typename as a property key and as a selected field:

types.union<UserOutput>()({
  User: {
    __typename: types.constant('User'),
    username: types.string(),
    body: types.string(), // GOOD: the compiler does complain about this
    foobar: types.string(), // GOOD: the compiler does complain about this
  },
  Post: {
    __typename: types.constant('Foobar'), // GOOD: the compiler does complain about this
    title: types.string(),
  },
});

I wanted to see if I can get around the stutter issue, but I haven't been successful. The reason why I chose an API of an object of { [__typename]: fields } is because I wanted the compiler to guarantee that each union member was included exactly once. I can't do this with array types.

What are your thoughts on the last example? Is this an acceptable API? I think I could make it backwards compatible by defaulting the union parameter to any.

edit: I intend for this to be implemented in the existing onUnion helper.

@acro5piano
Copy link
Owner

A happy new year from Japan 🎉

edit: I intend for this to be implemented in the existing onUnion helper.

Any changes that keeps the current API and improves type safety are welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants