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

Merge all GraphQL types? #113

Closed
dotansimha opened this issue Dec 27, 2017 · 8 comments
Closed

Merge all GraphQL types? #113

dotansimha opened this issue Dec 27, 2017 · 8 comments
Labels
🎙️question Further information is requested

Comments

@dotansimha
Copy link
Collaborator

I was wondering why this package merges (without extend type) only Query, Mutation and Subscription. The logic I refer to is this: https://github.com/okgrow/merge-graphql-schemas/blob/master/src/merge_types.js#L8

I tried to create a version of this package without this constraint and everything works, and I can just write type User { A: String } and merge it with type User { B: String } (without the need for extend type).

@RichardLitt RichardLitt added the 🎙️question Further information is requested label Dec 29, 2017
@cfnelson
Copy link
Contributor

cfnelson commented Jan 2, 2018

@dotansimha The merge-graphql-schemas pkg was written to support merging types before the usage/adoption of extend (last I checked extend is still undocumented). This package should also support merging Input types see here.

What other types are you trying to merge? And we are always happy to accept PR's to improve/maintain this package. 😄

Maybe @RodMachado the original author might be able to better answer your question.

@dotansimha
Copy link
Collaborator Author

@RodMachado @cfnelson
I think we can merge everything.
Because merging type User { id: String } and type User { username: String } could be very helpful and helps to create much modular GraphQL server.
Also, I think that Enum values can be merged, and GraphQL directives can be merged together.

@cfnelson
Copy link
Contributor

@dotansimha I believe this has been discussed at some point in the past but am unsure of the details. Will need to ask @RodMachado to clarify if there was a discussion and the outcome of it.

If I understand your suggestion correctly my main concern and I assume there will always be a contentious issue around what the expected behaviour should be and if we should allow/support resolving two of the same types together which would allow a scenario where you are trying to merge type User { id: String! } in fileA and type User { id: ID! } in fileB together and having to decide on how to handle/resolve any Field Definition conflicts.

@RodMachado
Copy link
Contributor

RodMachado commented Jan 12, 2018

Hey @dotansimha,

@cfnelson is right, we did have this discussion before and decided not to support multiple declarations for the same type.

Consider a scenario where there's a file A with type User { id: String } and a file be with type User { id: Int }.

Another scenario could be having type Address { zipcode: String } in file A and type Address { postalcode: String } in file B.

I know these are not the most elaborate examples, but I'm sure you can see how these could lead to some hard to debug or maintainability issues. Isn't it better to know that someone already created a type and that all fields are specified the same place?

Give it a thought. Maybe there are scenarios where this could be beneficial that we are not considering. I would love to hear your opinion.

Cheers!

@kohlikohl
Copy link

I would have following use case for merging every type together:
When using apollo-client and with the @client directives (which lets you create a client side field and resolver that does not exist on the graphql server), I would want to be able to combine a server schema with a client schema (being a superset of the server schema). I'd then use the resulting schema with apollo-codegen to generate interfaces for Typescript.

Granted, this is quite specific but with the rise of apollo client and typescript I can see this becoming a feature more devs would want.

Regarding the edge cases mentioned by @RodMachado: Why not just throw an error when finding fields with the same name that have different types and let the dev resolve it.
I don't see why the same field of the same type should be able to have two different data types.

@majelbstoat
Copy link

majelbstoat commented Feb 22, 2018

I don't know @kohlikohl (yet? 😉) , but it appears we've had the same requirements at the same time :) would love to voice support for the @client/apollo-codegen/typescript use-case.

@kohlikohl
Copy link

So I tried the code from PR #118 and it does exactly what I need.

@cfnelson
Copy link
Contributor

merge-graphql-schemas v1.5.0 🎉 has just been released which includes the ability to merge a GQL Type defined multiple times in separate files. Will throw an error when fieldDefintitons have conflicting values defined. Usage mergeTypes(types, { all: true }). Many thanks to @squidfunk work for making this possible. To see more check out PR #118.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎙️question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants