Skip to content
This repository has been archived by the owner on Apr 15, 2020. It is now read-only.

Type TypeVisitor is incorrect #14

Closed
AndKiel opened this issue Aug 9, 2019 · 9 comments
Closed

Type TypeVisitor is incorrect #14

AndKiel opened this issue Aug 9, 2019 · 9 comments

Comments

@AndKiel
Copy link

AndKiel commented Aug 9, 2019

I'm trying to implement a custom Transform based on the ones in /src/transforms to hide some stuff away. I saw that I should be able to do that by returning null in visitSchema (it's how it's done in FilterTypes). Unfortunately TypeVisitor won't allow me to do that because it is defined as returning only GraphQLNamedType. I'm not sure how it's possible that FilterTypes is working just fine with that declaration. It seems like | undefined | null is missing from type declaration.

@yaacovCR
Copy link
Owner

yaacovCR commented Aug 9, 2019

Looks to me like magic happening here:

https://github.com/yaacovCR/graphql-tools-fork/blob/master/src/transforms/visitSchema.ts#L60

But I agree that should be in TypeVisitor definition!

Will update as soon as I can. Feel free to submit pull request.

@yaacovCR
Copy link
Owner

Actually, how are you accomplishing this? visitSchema is not exported. Would part of this change involve exporting visitSchema and its dependent types?

@AndKiel
Copy link
Author

AndKiel commented Aug 12, 2019

Import hints told me to do:

import { visitSchema, VisitSchemaKind } from "graphql-tools-fork/dist/transforms/visitSchema"; 

and it works, type is exported/visible from there.

@yaacovCR
Copy link
Owner

yaacovCR commented Aug 14, 2019

Sounds like you are importing from non package exports. What you really are requesting is that visitSchema be exported and types touched up.

See ardatan#1070
And ardatan#922

What I am not sure about is whether visitSchema and SchemaVisitor can/should be consolidated...

@yaacovCR
Copy link
Owner

yaacovCR commented Sep 1, 2019

Finished initial review of all the ways graphql-tools modifies schemas, see #19. My plan as of now is to deprecate the visitSchema in the transforms section and rewrite the individual transforms with direct modification or SchemaVisitor pattern.

But I will probably export both visitSchemas for backward compatibility, not sure about the names to use, thoughts?

@AndKiel
Copy link
Author

AndKiel commented Sep 1, 2019

Current names sound fine to me as they are tied to the code responsibility and clear, visitSchema/SchemaVisitor visits the "nodes" inside the schema, transformSchema/SchemaTransform changes those "nodes" (based on the visitation results, because they are kinda tied together, if I understand correctly?). Not sure if there are better names for this.

@yaacovCR
Copy link
Owner

yaacovCR commented Sep 1, 2019

@yaacovCR
Copy link
Owner

yaacovCR commented Sep 9, 2019

Working on a new visitSchema function that integrates both, i.e., can take either a visitorSelector function that returns an array of SchemaVisitor instances or an object with VisitSchemaKind keys. Hopefully, the underlying implementations can be unified as well.

See #20.

@yaacovCR
Copy link
Owner

Should be fixed now, have to work on docs for consolidated visitSchema, but existing transforms should "just work" with the caveat that the type for the object with VisitSchemaKind keys is now called SchemaVisitorMap so as not to conflict with the SchemaVisitor class-based version.

The consolidated visitSchema is now exported.

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

No branches or pull requests

2 participants