amount / gql2ts

convert a graphql schema to a typescript definition
MIT License
25 stars 4 forks source link

A union of object interfaces is formatted oddly with inline fragments #261

Open dannycochran opened 5 years ago

dannycochran commented 5 years ago

Related issues: https://github.com/avantcredit/gql2ts/issues/248

Given the following GraphQL schema:

type AssetInfo {
  id: String!
  data: AssetData
}

union AssetData = Video | Audio | Artwork

type Video {
  subType: String
}

type Audio {
  identifiers: [String]
}

type Artwork {
  tags: [String]
}

And this query against it:

{
  id
  data {
    ... on Artwork {
     tags
   }
   ... on Audio {
     identifiers
   }
   ... on Video {
     subType
   }
  }

gql2ts produces the following response for that query:

interface SelectionOnAssetInfo {
  id: string;
  data:
    | Partial<SpreadOnArtwork> &
        Partial<SpreadOnCopy> &
        Partial<SpreadOnVideo>
    | null;
}

I would expect the corresponding query to produce this interface

interface SelectionOnAssetInfo {
  id: string;
  data: SpreadOnArtwork | SpreadOnCopy | SpreadOnVideo | null;
}
  1. The data field should not be wrapped with Partial
  2. The interfaces should not be intersected with &, but unioned via |.
brettjurgens commented 5 years ago

@dannycochran yeah, this is what I was working on in #195. can you try using v2.0.0-3? It changes the names of the generated interfaces, so not entirely backwards compatible, but it's more accurate.

dannycochran commented 5 years ago

@brettjurgens I'm unable to install at v2.0.0-3.

npm install --save-dev git://github.com/avantcredit/gql2ts.git#v2.0.0-3

produces this error:

npm ERR! enoent ENOENT: no such file or directory, chmod '/documents/myapp/node_modules/gql2ts-base/index.js'

I'm guessing there's some publishing / build steps that that tag is missing?

brettjurgens commented 5 years ago

@dannycochran would it be possible to install via npm instead of git? i don't know if installing via git will work since it's a lerna repo ("gql2ts-base" in the error is the root package.json which doesn't contain anything useful for end-users)

you can use either the version number or the @next tag on npm

npm install --save-dev gql2ts@next @gql2ts/from-query@next  # whichever package you need/want
dannycochran commented 5 years ago

@brettjurgens that worked, but I think the new update is having some issues. I now get an error when running fromQuery for my first query:

Cannot read property 'operation' of undefined

It looks like it's failing on this fragment, which is the first I try to pass in to fromQuery (I loop through all of them):

export const BaseTitleInfoFragment = gql`
  fragment BaseTitleInfo on Movie {
    movieId
  }
`;

const queryResults = fromQuery(schema, BaseTitleInfoFragment.loc!.source.body);
brettjurgens commented 5 years ago

hmm...

it looks like you can't generate from just a fragment 😞

I was able to get it working with:

const schema = `
  type Query {
    movie: Movie
  }

  type Movie {
    movieId: ID!
    title: String!
  }
`;

const query = `
  query {
    movie {
      ...BaseTitleInfo
    }

  }
  fragment BaseTitleInfo on Movie {
    movieId
  }
`;

const queryResults = fromQuery(schema, query);

I'll look into supporting just fragments (I don't know off-hand why it doesn't work)

Will this work-around work in the meantime?

it's possible to inline all of the fragments you use in the query and run through fromQuery

dannycochran commented 5 years ago

@brettjurgens I can inline all of the fragments / queries and get fromQuery to work, but it looks like the new signature response for fromQuery has changed to return a string. It used to return IFromQueryReturnValue.

The newly produced string is also missing a lot of my queries and types. It's also no longer creating distinct enums or union values, but inlining them into the type definition. Example:

export interface SelectionOnMovie {
  movieId: number;
  type:
    | 'Standalone'
    | 'Show'
    | null;
  title: string | null;
}

That type field is actually a graphql enum in the underlying schema:

enum MovieType {
  Standalone
  Show
}

It used to be broken out into its own enum in 1.9.0. Is there a method that looks more like the one from 1.9.0? This is how I used to write out my file:

const typescriptDefinitions = Object.keys(AppQueries).reduce<Set<string>>((defs, exportName) => {
    const query: DocumentNode = (AppQueries as { [queryName: string]: DocumentNode })[exportName];
    let queryResults: IFromQueryReturnValue[] = [];
    try {
      queryResults = fromQuery(schema, query.loc!.source.body, undefined, {
        generateFragmentName: fragmentName => fragmentName,
      });
    } catch (err) {
      throw new Error(`Failed to generate typings for ${exportName}: ${err.message}`);
    }
    // Add variables, interfaces, and additionTypes to a set to prevent duplicates.
    queryResults.forEach(query => {
      defs.add(query.variables);
      defs.add(query.interface);
      query.additionalTypes.forEach(t => defs.add(t));
    });
    return defs;
  }, new Set());
brettjurgens commented 5 years ago

but it looks like the new signature response for fromQuery has changed to return a string. It used to return IFromQueryReturnValue.

Yeah this was done to simplify the return value. At the time of writing, I didn't see a case where it was useful to split them up - perhaps this was misguided. Do you need this functionality? Duplicates shouldn't be an issue

The newly produced string is also missing a lot of my queries and types.

hmm... this shouldn't be happening. Do you have a minimal example that I can look at to test?

It's also no longer creating distinct enums or union values, but inlining them into the type definition

Yeah, this was intentional. the background for this is in #179. The gist of it and the conclusion I drew from it was to use string literal unions in from-query and use enums in from-schema. We had run into several issues with using native enums (both enum & const enum). The most notable one was that 2 enums that are the same... aren't. so:

enum MovieType {
  Standalone = 'Standalone',
  Show = 'Show'
}

enum MovieType2 {
  Standalone = 'Standalone',
  Show = 'Show'
}

MoveType.Standalone !== MovieType2.Standalone

It does make sense, two enums should not be the same but also doesn't because the value 'Standalone' should equal 'Standalone'. This is a problem because if you have two queries/mutations that rely on the same enum, the generated one will be different and you'll get a compiler error if you share values between the two.

I had run into this a few times and couldn't figure out a suitable workaround before deciding that string literals were the way to go.

We also generated the schema definitions using from-schema and imported the enums from that file. So we had something like the following:

import { MovieType } from 'schema.ts';
import { SelectionOnMovie } from './query.graphql';

const myObject: SelectionOnMovie = {
  movieId: 123,
  type: MovieType.Standalone,
  title: 'Avengers'
};
dannycochran commented 5 years ago

Yeah this was done to simplify the return value. At the time of writing, I didn't see a case where it was useful to split them up - perhaps this was misguided. Do you need this functionality? Duplicates shouldn't be an issue

Duplicates are an issue with the current version, but yeah if they're no longer an issue I'm fine with just the string signature.

It's also no longer creating distinct enums or union values, but inlining them into the type definition

This makes sense and I can adopt this new pattern per your comments.

hmm... this shouldn't be happening. Do you have a minimal example that I can look at to test?

The error I'm getting is Error: InlineFragment Must Be Inlined!. What's interesting is I only get this error when running fromQuery on just this query. When I combine all my queries into a single string and run fromQuery, I don't get an error and the results of the query with inline fragments just don't show up.

Here's a sandbox showing the error though: https://codesandbox.io/embed/gracious-lumiere-dt6e3

brettjurgens commented 5 years ago

The error I'm getting is Error: InlineFragment Must Be Inlined!. What's interesting is I only get this error when running fromQuery on just this query.

Thanks for the sandbox! I think it's because SpecData is a union type and I might not have built out support 100% for those 😞 (for context when i was at avant, we typically used interfaces, so I didn't have much test data). Indeed, changing the union to an interface makes the query compile. When I can find some time, I'll add support for unions. (It looks like I left myself a TODO for that 😄 https://github.com/avantcredit/gql2ts/blob/experiment_with_fragments/packages/from-query/src/ir.ts#L168)

When I combine all my queries into a single string and run fromQuery, I don't get an error and the results of the query with inline fragments just don't show up.

I don't remember 100%, but I don't think fromQuery supports multiple queries in one string.