Graphene: Implementation of Connections somewhat broken in 1.0

Created on 28 Sep 2016  路  4Comments  路  Source: graphql-python/graphene

First of all, excited to see 1.0 released! But the way Connections have been released does not conform to the spec and also regressed in functionality compared to 0.9, by not having this test: https://github.com/graphql-python/graphene/blob/0.x/graphene/relay/tests/test_query.py#L69

Having this check: https://github.com/graphql-python/graphene/blob/master/graphene/relay/connection.py#L111-L114 in the ConnectionField makes it out of the box not work with ObjectTypes, which per spec are allowed to be used as Connections. My PR https://github.com/graphql-python/graphene/pull/288 addresses this issue. This is important to us because with how things are right now we can not upgrade our apps and we would love to do this as soon as possible.

Also a connection field resolver still does not allow the returning of a Promise.

Thanks for all the great work done so far!

Most helpful comment

I just added support to return a Connection instance in the connection resolver.

https://github.com/graphql-python/graphene/commit/c7929234294a181993158b4439c24006e9ed1ebd

All 4 comments

Hey Markus,

Thanks for opening the PR and this issue and sorry for replying so late.
I agree that the Connections implementation have a lot of things to improve, and actually I like most of the ideas that you have in the PR.

However before moving forward with it I think is worth to analyze how connections should work, trying to solve questions like:

  • Where the connection resolution should live? Should we start transferring some resolver logic to the Connection type?
  • How we should handle the "automatic" connection types creation? We should handle it at all? (as is right now)
  • How we can handle different connections for same types? And then, how we handle name clashing?

Some of this questions are easy to solve if we are more explicit (as your PR points), but before choosing a definitive approach would be good to explore if there is any way that we could achieve simplicity and extensibility at the same time (as we did for Nodes).

In general I think is worth to spend some time analyzing, discussing and getting a stable API for connections than releasing something that we might have to change after.

Please raise any ideas you have! (and thanks for your great work too!)

PS: Meanwhile I'm going to add support to the test you pointed to unblock your transition to 1.0 :)

Hey Syrus,

Thank you for getting back with such a detailed answer. Yes, my PR is certainly not perfect, it was partially also me just getting to know that area of the code and if I would make another PR I would probably do it differently.

And yes, I think it is very good that you are asking these questions and before anything is rewritten we should answer them.

  • Having some connection resolution on the type could make sense. Was there anything you had in mind already?
  • There more I think about it, the more I am against the 'automatic' connection type creation. It is really cool to start off with, but for a slightly bigger sized team, where you might have other people come on/off it has been less maintainable in my experience. You have to explain people that this does not return the type itself, but something wrapped around it etc. And imo while it does make code writing easier it makes its maintenance more difficult (because of the magic one has to be aware of). I think just making it easy to create and return connection types would be the best of both worlds: You have to write a bit more but there is no magic going on and one can understand everything just from reading the app code.
  • If we don't have the 'automatic' creation anymore it should be easier to have different connections for the same type: The user is always explicitly creating a connection so if subclassing from Connection then creating giving it whatever name they want. If we would have something like DefaultConnection.for_type(Type) then you could only create that once per schema.

Let me know what you think. I would be happy btw to create a fresh PR based on the ideas we come up with here.

I just added support to return a Connection instance in the connection resolver.

https://github.com/graphql-python/graphene/commit/c7929234294a181993158b4439c24006e9ed1ebd

Hi @Globegitter . We're currently going through old issues that appear to have gone stale (ie. not updated in about the last 6 months) to try and clean up the issue tracker. If this is still important to you please comment and we'll re-open this.

Thanks!

Was this page helpful?
0 / 5 - 0 ratings

Related issues

mraak picture mraak  路  3Comments

danpalmer picture danpalmer  路  4Comments

defrex picture defrex  路  3Comments

junchiz picture junchiz  路  3Comments

fishy picture fishy  路  4Comments