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

Makes PageInfo and Edge public to make custom connections easier #107

Merged
merged 4 commits into from
Mar 13, 2023

Conversation

cshadek
Copy link
Contributor

@cshadek cshadek commented Mar 8, 2023

This is a really small change, but it makes several Connection related types public including PageInfo, Edgeable, and Edge, so they can be used in custom connections.

My main concern is PageInfo because I want to create a custom connection, and right now PageInfo is internal and gets added to the schema automatically when using the default connections. I figured Edgeable and Edge may be useful too. It's possible much more of the Connection API could be made public as well.

Are there any downsides to doing this?

@NeedleInAJayStack
Copy link
Member

No huge downsides - this pattern follows the relay's pagination spec, which seems pretty stable and well-known. That said, making these public does increase our public API surface area.

Could you explain your use case a little more? Are you reaching for these types because ConnectionType is experiencing the bug logged here? Or is your custom connection doing something more complex that ConnectionType can't support?

@cshadek
Copy link
Contributor Author

cshadek commented Mar 9, 2023

My main motivation is to create custom connections and edges with additional fields, but still be able to leverage some of the Graphiti connections code.

For example, I'd like a connection to have a totalCount field.

As for edges, I could see a use case like described here

Some of the slicing related code code could also be useful to be public, but I started with the minimal amount.

@cshadek
Copy link
Contributor Author

cshadek commented Mar 9, 2023

It's possible there's a better way to do this without exposing more of the API. There are other issues with the connections implementation too, such as you can't do a connection of a interfaces or union. I've gotten around this by creating a wrapper type to serve as the Node, but this is not ideal long term.

In summary, it would be useful if connections allowed more customization and also allowed protocols as the node.

@cshadek
Copy link
Contributor Author

cshadek commented Mar 9, 2023

Another example of custom connections is the Github GraphQL API.

They tend to add extra fields for most of their connections and edges. The thing is though the slicing and cursor logic would still be the same as the default connection type. Same with the PageInfo type. @NeedleInAJayStack what are your thoughts on this?

@NeedleInAJayStack
Copy link
Member

@cshadek Cool, thanks for the info. I just worked through some of the details of supporting custom fields on these types and yeah, I think we'll probably want to expose these as public. Connections have been under-tested and under-documented in this package, so I'll be adding more of that soon. Thanks!

@NeedleInAJayStack NeedleInAJayStack marked this pull request as ready for review March 12, 2023 21:46
Copy link
Member

@NeedleInAJayStack NeedleInAJayStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also make the edges and pageInfo properties on Connection public?

Sources/Graphiti/Connection/Edge.swift Outdated Show resolved Hide resolved
cshadek and others added 2 commits March 12, 2023 16:58
Co-authored-by: Jay Herron <30518755+NeedleInAJayStack@users.noreply.github.com>
@cshadek cshadek changed the title Make PageInfo, Edgeable, and Edge public to make custom connections more easier Makes PageInfo and Edge public to make custom connections easier Mar 12, 2023
@cshadek
Copy link
Contributor Author

cshadek commented Mar 13, 2023

@NeedleInAJayStack I added your suggestions. I think it should be ready to merge.

@NeedleInAJayStack NeedleInAJayStack merged commit c6e8d4d into GraphQLSwift:main Mar 13, 2023
@cshadek cshadek deleted the custom-connections branch December 1, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants