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

Allow schemaPrinter to be customized / extended. #2020

Open
j opened this issue Jul 8, 2019 · 12 comments
Open

Allow schemaPrinter to be customized / extended. #2020

j opened this issue Jul 8, 2019 · 12 comments

Comments

@j
Copy link

j commented Jul 8, 2019

Some libraries are having to copy the schemaPrinter code to do custom stuff to the output, (@apollo/federation, type-graphql). These libraries require outputting type and field level directives since this isn't supported in this library. Is there any support to do so, or can the schemPrinter just export the other functions so we don't have to copy every function or, even better, create a SchemaPrinter class so everything can be over-ridden, then printSchema options can set the printer for BC:

type Options = {
    commentDescriptions?: boolean,
    printer?: SchemaPrinter
};

const defaultPrinter = new SchemaPrinter();

export function printSchema(schema: GraphQLSchema, options?: Options): string {
    const printer = options.printer || defaultPrinter;

    return printer.print(
        schema,
        n => !isSpecifiedDirective(n),
        isDefinedType,
        options,
    );
}

export function printIntrospectionSchema(
    schema: GraphQLSchema,
    options?: Options,
): string {
    const printer = options.printer || defaultPrinter;

    return printer.print(
        schema,
        isSpecifiedDirective,
        isIntrospectionType,
        options,
    );
}
@mjmahone
Copy link
Contributor

mjmahone commented Jul 8, 2019

Instead of making an extensible printer, would it solve your issue if directives on schema definitions were included in the print step?

I believe this is actually a bug where we forgot to add directive printing for printSchema when we added directives on type and field definitions. I'd welcome both a PR for test cases that are currently missing and a PR for fixing the issue in the printer.

@IvanGoncharov
Copy link
Member

@j @mjmahone I don't think we should print directives based on astNode properties since directives are tight to SDL context. For example:

type Foo {
  foo: String
}

type Bar @copyFields(fromType: "Foo") {
  bar: String
}

Assuming @copyFields directive successfully modified the schema and assuming we implemented your proposal output will be like that:

type Foo {
  foo: String
}

type Bar @copyFields(fromType: "Foo") {
  foo: String
  bar: String
}

That doesn't make sense in the context of @copyFields or any other directive that do something more complicated than attaching metadata.

I think the correct solution would be to design GraphQLSDLDirective (derived from GraphQLDirective) and allow you to register callbacks for transforming types/fields (including extensions as described in #1527) and another callback that called during schema printing where directive can decide to return DirectiveNode for particular type/field (probably base on extensions).

This solution also solves the problem of code-first schema that wants to be printed with additional data represented as directives in SDL.
So I propose to freeze this issue until we start 16.0.0-alpha.x (~1 month) so we would be able to experiment with API design without breaking changes.

@IvanGoncharov IvanGoncharov added this to the 16.0.0-alpha.1 milestone Jul 9, 2019
@j
Copy link
Author

j commented Jul 9, 2019

@IvanGoncharov

That definitely sounds like a much better solution. I was just noticing other libraries are simply copy/pasting schema printer to do their own logic and pull out directives as their library has them. extensions sounds elegant as well as the GraphQLSDLDirective. Combined pretty much solves everyone's issues. extensions alone will fix the code-first schema for many libraries creating schema using GraphQLSchema instead of SDL.

I'm super excited for this change as it'll marry everyone's wishes (having directives :P)

I've been working on implementing apollo-federation into type-graphql and they use astNode to do field-level directives. extensions will make it all feel less hacky.

Ref: MichalLytek/type-graphql#369 apollographql/apollo-server#3013

@vicary
Copy link

vicary commented Aug 19, 2019

Found this after I made appsync-schema-convert myself, also copying schemaPrinter.js.

Looking forward to 16.0.0 for a more elegant solution!

@mpiroc
Copy link

mpiroc commented Jan 1, 2020

@IvanGoncharov What if printSchema only printed directives that were not visited during schema construction? Using your example:

type Foo {
  foo: String
}

type Bar @copyFields(fromType: "Foo") {
  bar: String
}
// 1
printSchema(makeExecutableSchema({
    typeDefs,
    schemaDirectives: {
        copyFields: CopyFieldsVisitor
    }
}))

// 2
printSchema(makeExecutableSchema({
    typeDefs,
    schemaDirectives: {
    }
}))

The first snippet would print:

type Foo {
  foo: String
}

type Bar {
  foo: String
  bar: String
}

The second would print:

type Foo {
  foo: String
}

type Bar @copyFields(fromType: "Foo") {
  bar: String
}

@barbieri
Copy link

At least printing the directives would help me immensely, thins like input field, field, object, input object declarations for validation and permission annotations via directives would just work. But if we need to provide a custom printer, that's also ok.

@spawnia
Copy link
Member

spawnia commented Jun 23, 2021

How about we add a config option:

printDirectives: (definition: GraphQLType|GraphQLField|GraphQLEnumValue|GraphQLInputField|GraphQLArgument) => string

Whenever a definition that can hold directives is encountered, the given function would be called with it and is free to print any directives.

@vicary
Copy link

vicary commented Jun 27, 2021

@spawnia Will it by any chance be possible that I can make use of the utilities available internally to schemaPrinter, such that I can be confident that output formats are consistent with other types of blocks?

@julrich
Copy link

julrich commented Jul 8, 2021

I'd also be really interested in a solution here! Stumbled upon #2145 (comment) while searching around, which seems to at least offer a workaround (will have to test that).

And I found this comment, which received quite some upvotes: #869 (comment)

Would that still be the way to "polyfill" this when printing a schema? Use-case here is printing a "Gatsby-compatible" schema, which includes adding some directives used by Gatsby (like @fileByRelativePath).

@vicary
Copy link

vicary commented Jul 8, 2021

@julrich My appsync-schema-converter have that in mind, originally designed for @aws* directives but it may also serves your purpose. You may see the includeDirectives and directiveFilter option for more details.

@julrich
Copy link

julrich commented Jul 8, 2021

I'll have look, thanks! But I'm not sure that it would address my use-case.
I'm not starting from a GraphQL schema, but from JSON Schemas. I'd add the extensions.directives as needed while converting (e.g. add @fileByRelativePath for every schema field with type -> string and format -> image).

@vicary
Copy link

vicary commented Jul 8, 2021

Oh I thought schema.buildObjectType(...) is not related to graphql-js and automatically assumed you must be using the SDL approach, apologies for not super familiar with Gatsby.

spawnia added a commit to spawnia/graphql-js that referenced this issue Nov 5, 2021
Resolves graphql#2020

I initially thought about implementing this as graphql#2020 (comment),
but really could not think of a good use case where
a directive should be printed in a modified way.

Thus, I went for a simpler signature:

```ts
shouldPrintDirective?: (directiveNode: DirectiveNode) => boolean
```

This is only a partial implementation for now, there are more possible
locations in a schema that can have a directive. Before I finish the
implementation, I would like to validate the direction is right.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants