-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Comments
WIP research results The good: The not so good: Summary so far: |
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.
The issue linked in that comment is graph-gophers/graphql-go#241. @sourcegraph/cloud do you have thoughts on this?
Intuitively I don't see why that would be the case, but maybe I'm missing something. |
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 :) |
Upd:
// 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
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. |
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?) |
copy pasting === inlining, which is our workaround today. We cannot use fragments but a normal query is OK. @felixfbecker or you were asking about something else? |
Yes a normal query should work :) not sure why I didn’t take that tradeoff🤔 |
Ok I made a plugin that generates our interface thing. 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. |
Awesome! When's the PR coming? 🙂 |
today, hopefully |
@felixfbecker Can this be closed? |
Yup |
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.tsThe 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.
The text was updated successfully, but these errors were encountered: