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

Replace forked ts-graphql-plugin-based type query generator #12177

Closed
felixfbecker opened this issue Jul 14, 2020 · 12 comments
Closed

Replace forked ts-graphql-plugin-based type query generator #12177

felixfbecker opened this issue Jul 14, 2020 · 12 comments
Assignees
Labels
testing Issues that deal with unit tests, integration tests and the testing infrastructure.

Comments

@felixfbecker
Copy link
Contributor

For integration tests, we currently use a mostly-copy-pasted adapted version of ts-graphql-plugin: https://github.com/sourcegraph/sourcegraph/blob/master/shared/dev/gql2ts-transformer.ts

The generator has multiple bugs in the typings it generates (see casts in integration tests), but lacks comments and is hard for us to maintain. It would be better to rely on a better-working open source tool, e.g. GraphQL code generator which could be combined with graphql-tag-pluck to extract the types out of our TypeScript code.

The current bugs also likely make it not useable for the general use case of typing queries.

@felixfbecker felixfbecker added team/web testing Issues that deal with unit tests, integration tests and the testing infrastructure. labels Jul 14, 2020
@twop
Copy link
Contributor

twop commented Jul 15, 2020

WIP research results

The good:
GraphQL code generator can potentially do 90% of what we need today for integration tests specifically
It can also replace our current codegen gql2ts because it covers that usecase too
It has watch mode
writing our own plugin to generate a big interface for gql operations seems doable
It has graphql-tag-pluck built in

The not so good:
graphql-tag-pluck has several issues with our current code base and I don’t know the scope of the problems yet. One of them: https://sourcegraph.com/github.com/sourcegraph/sourcegraph@870221e07c86de5e67d3900a804ce6547b595fd3/-/blob/web/src/search/backend.tsx#L16:7
it requires unique fragment names.
I’m not sure yet if it^ implies that we cannot reuse fragments across queries.

Summary so far:
It seems like the right tool for the job
I will continue to invest time to make it work

@felixfbecker
Copy link
Contributor Author

it requires unique fragment names.

This doesn't seem like a problem - just like unique query names, unique fragment names have other benefits, e.g. you can easily grep/sourcegraph for them to find them in the codebase.

graphql-tag-pluck has several issues with our current code base and I don’t know the scope of the problems yet. One of them: sourcegraph.com/github.com/sourcegraph/sourcegraph@870221e07c86de5e67d3900a804ce6547b595fd3/-/blob/web/src/search/backend.tsx#L16:7

The issue linked in that comment is graph-gophers/graphql-go#241. @sourcegraph/cloud do you have thoughts on this?
cc @eseliger who wrote that comment.

I’m not sure yet if it^ implies that we cannot reuse fragments across queries.

Intuitively I don't see why that would be the case, but maybe I'm missing something.

@twop
Copy link
Contributor

twop commented Jul 15, 2020

Good news on Fragments reuse

this code works:

import gql from 'graphql-tag';

const SharedUser = gql`
  fragment SharedUser on User {
    id
    username
  }
`;

export const QueryA = gql`
  query findUserA($userId: ID!) {
    user(id: $userId) {
      ...SharedUser
    }
    ${SharedUser}
  }
`;

const SharedUserDup = gql`
  fragment SharedUser on User {
    id
    username
  }
`;

export const QueryB = gql`
  query findUserB($userId: ID!) {
    user(id: $userId) {
      ...SharedUser
    }
    ${SharedUserDup}
  }
`;

But changing it too

const SharedUserDup = gql`
  fragment SharedUser on User {
    id
    username
    someOtherField <-----
  }
`;

will result in an error.

It seems that the tooling checks structural equality vs picking just declarations from code, which is awesome!

So as long as unique names represent different structures we are free to organize code however we want :)

@twop
Copy link
Contributor

twop commented Jul 15, 2020

Upd:

  1. custom gql tag works with no extra configuration (our usecase)
  2. Confirmed that string concatenation breaks the tooling
// Not using "graphql-tag" is fine
import { gql } from './gql';

const SharedUser = `
    id
    username
`;
// const SharedUser = gql`
//   fragment SharedUser on User {
//     id
//     username
//   }
// `;

export const QueryB = gql`
  query findUserB($userId: ID!) {
      user(id: $userId){
        ${SharedUser} <---- this breaks the tooling
      }
  }
`;

Error output

    GraphQLError: Syntax Error: Expected Name, found "}".
        at syntaxError (/Users/twop.sk/work/gql-stub/node_modules/graphql/error/syntaxError.js:15:10)
        at Parser.expectToken (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:1423:40)
        at Parser.parseName (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:92:22)
        at Parser.parseField (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:289:28)
        at Parser.parseSelection (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:278:81)
        at Parser.many (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:1537:26)
        at Parser.parseSelectionSet (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:265:24)
        at Parser.parseField (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:306:68)
        at Parser.parseSelection (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:278:81)
        at Parser.many (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:1537:26)
        at Parser.parseSelectionSet (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:265:24)
        at Parser.parseOperationDefinition (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:193:26)
        at Parser.parseDefinition (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:131:23)
        at Parser.many (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:1537:26)
        at Parser.parseDocument (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:109:25)
        at Object.parse (/Users/twop.sk/work/gql-stub/node_modules/graphql/language/parser.js:36:17)

So I think it might make sense just to copypaste the shared fragment with a comment "copied because of X", not ideal but probably not as critical.

@felixfbecker
Copy link
Contributor Author

As I understand it, copy-pasting the fragment wouldn't solve this panic issue: https://sourcegraph.com/github.com/sourcegraph/sourcegraph@870221e07c86de5e67d3900a804ce6547b595fd3/-/blob/web/src/search/backend.tsx#L16:7 (or are we talking about another issue?)
If there is only one case where we're not using a proper fragment because of a backend bug, we could also just not use the gql tag there.

@twop
Copy link
Contributor

twop commented Jul 15, 2020

copy pasting === inlining, which is our workaround today. We cannot use fragments but a normal query is OK.
@eseliger is that true?

@felixfbecker or you were asking about something else?

@eseliger
Copy link
Member

Yes a normal query should work :) not sure why I didn’t take that tradeoff🤔

@twop
Copy link
Contributor

twop commented Jul 15, 2020

Ok I made a plugin that generates our interface thing.
The best part that this is pretty much it:

import { Types, PluginFunction } from '@graphql-codegen/plugin-helpers';
import { GraphQLSchema, concatAST, visit } from 'graphql';
import { capitalCase } from 'change-case';

type Operation = {
  type: 'query' | 'mutation' | 'subscription';
  name: string;
};

export const plugin: PluginFunction<{}, string> = (
  schema: GraphQLSchema,
  documents: Types.DocumentFile[],
  config: {}
) => {
  // const identifierName = config.identifierName || 'namedOperations';
  const allAst = concatAST(documents.map(v => v.document));
  const allOperations: Operation[] = [];

  visit(allAst, {
    enter: {
      OperationDefinition: node => {
        if (node.name?.value) {
          allOperations.push({ type: node.operation, name: node.name.value });
        }
      },
    },
  });

  const interfaceFields = allOperations.map(({ type, name }) => {
    const fullname = `${capitalCase(name, { delimiter: '' })}${capitalCase(
      type
    )}`;
    return `${name}: (variables: ${fullname}Variables) => ${fullname};`;
  });

  if (interfaceFields.length === 0) {
    // eslint-disable-next-line no-console
    console.warn(
      `Plugin "named-operations-object" has an empty output, since there are no valid operations!`
    );

    return '';
  }

  return `export interface AllOperations {
${interfaceFields.join('\n')}
}`;
};

It outputs this code:

export interface AllOperations {
findUserA: (variables: FindUserAQueryVariables) => FindUserAQuery;
findUserC: (variables: FindUserCQueryVariables) => FindUserCQuery;
}

So yeah, super happy with it so far.

@felixfbecker
Copy link
Contributor Author

Awesome! When's the PR coming? 🙂

@twop
Copy link
Contributor

twop commented Jul 16, 2020

today, hopefully

@eseliger
Copy link
Member

@felixfbecker Can this be closed?

@felixfbecker
Copy link
Contributor Author

Yup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Issues that deal with unit tests, integration tests and the testing infrastructure.
Projects
None yet
3 participants