feat: Allow connection to be used without relay for generic cursor pagination #3669
Conversation
Reviewer's Guide by SourceryThis pull request moves the Connection implementation from the relay package to a new pagination package, allowing for more general-purpose cursor pagination. It introduces new imports from strawberry.pagination, deprecates some imports from strawberry.relay, and updates documentation and tests accordingly. The changes aim to make the Connection feature more accessible and flexible for use outside of the Relay specification context. Class diagram for the new pagination packageclassDiagram
class Connection {
+PageInfo page_info
+List~Edge~ edges
+resolve_node(node, info, kwargs) NodeType
+resolve_connection(nodes, info, before, after, first, last, kwargs) AwaitableOrValue~Self~
}
class ListConnection {
+PageInfo page_info
+List~Edge~ edges
+resolve_connection(nodes, info, before, after, first, last, kwargs) AwaitableOrValue~Self~
}
class PageInfo {
+bool has_next_page
+bool has_previous_page
+Optional~str~ start_cursor
+Optional~str~ end_cursor
}
class Edge {
+str cursor
+NodeType node
+resolve_edge(node, cursor) Self
}
Connection <|-- ListConnection
Connection o-- PageInfo
Connection o-- Edge
Edge --> NodeType
Class diagram for the updated relay packageclassDiagram
class GlobalIDValueError {
}
class NodeIDAnnotationError {
}
class NodeExtension {
}
class Node {
}
class GlobalID {
}
class NodeID {
}
GlobalIDValueError <|-- NodeIDAnnotationError
Node <|-- NodeExtension
Node <|-- GlobalID
Node <|-- NodeID
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bellini666 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
|
|
||
| def __getattr__(name: str) -> Any: |
There was a problem hiding this comment.
suggestion (performance): Consider performance impact of getattr
While using getattr for deprecation warnings is a clean approach, consider its potential impact on import times and overall performance. Consider profiling the import process to ensure this doesn't introduce significant overhead, especially for large projects.
def __getattr__(name: str) -> Any:
if name in _DEPRECATIONS:
return _lazy_load(name)
raise AttributeError(f"module '{__name__}' has no attribute '{name}'")
def _lazy_load(name: str) -> Any:
warnings.warn(_DEPRECATIONS[name], DeprecationWarning, stacklevel=2)
return importlib.import_module(f".{name}", __package__)
|
Thanks for adding the Here's a preview of the changelog: This release moves the The following now can be imported from
Those can still be used together with the relay package, but importing from it You can read more about connections in the Here's the tweet text: |
bellini666
force-pushed
the
generic_connection
branch
from
ddcc952 to
0c3d5bb
Compare
| @@ -0,0 +1,21 @@ | |||
| from .fields import ConnectionExtension, connection | |||
There was a problem hiding this comment.
Pagination and everything inside it is composed of Connection code moved from the relay package. No code here is new 😊
There was a problem hiding this comment.
mmh, I wonder about this, Connection is a relay specific term, no? I wonder if moving it outside will bring confusion 🤔
There was a problem hiding this comment.
I think relay coined that term, but today it is a general way of doing pagination in GraphQL. An example is this: https://graphql.org/learn/pagination
I have absolutely nothing against keeping it in relay, and calling it "relay style pagination" (as we were discussing in private =P), I'm only afraid of still making people confused, thinking this can only be used with relay.dev
Let me ping @strawberry-graphql/core here to get everyone's opinions :)
There was a problem hiding this comment.
I think relay coined that term, but today it is a general way of doing pagination in GraphQL. An example is this: https://graphql.org/learn/pagination
On the last section of this page it says:
To ensure a consistent implementation of this pattern, the Relay project has a formal specification you can follow for building GraphQL APIs which use a cursor based connection pattern.
We should, therefore, be very clear about what kind of connection/pagination types we want to provide.
Do we want to (1) provide a single Connection type that does not strictly follow any specification but can be used with and without Relay nodes? Or (2) do we want to provide two separate Connection types: One that strictly follows the "GraphQL Cursor Connections Specification" by Relay (i.e., the one where nodes need to be bound to Node) and another one that can be used with any type.
If we want a connection type that strictly follows the spec, we could still share the implementation internally but must ensure the nodes are still bound to Node.
There was a problem hiding this comment.
@DoctorJohn my idea in this PR was for (1), but if we go for (2), which is what @patrick91 was suggesting, we are going to limit it from being used with non Node objects, which is not something that the connection actually requires (but I got your point)
What do you think about scheduling a discussion on this on discord? @patrick91 is out right now but we can do that once he gets back :)
There was a problem hiding this comment.
for what is worth, I'm ok with dropping the requirement on Node, but still keep it as relay pagination 😊
CodSpeed Performance ReportMerging #3669 will not alter performanceComparing Summary
|
bellini666
force-pushed
the
generic_connection
branch
from
0c3d5bb to
e68edba
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3669 +/- ##
==========================================
+ Coverage 96.67% 96.98% +0.31%
==========================================
Files 503 511 +8
Lines 33457 33554 +97
Branches 5602 5625 +23
==========================================
+ Hits 32344 32544 +200
+ Misses 880 800 -80
+ Partials 233 210 -23 |
There's no need to force
Connectionto be relay specific, as it doesn't enforce the objects being paginated to implement theNodeinterface or even to useGlobalIDs.This is moving the
Connection/ListConnection/etc tostrawberry.pagination`, allowing it to be used without relay for general-purpose cursor pagination.This is something that we have been discussing for a while.
obs. I have not changed the implementation of anything other than removing the
boundfromNodeType, which should allowConnection[FooType]whenFooTypedoesn't implementNode. The changes seem massive but they're mostly moving code from one place to the other =Pobs2. I also kept the older names as aliases in the
relaypackage, but added a deprecation warning in case someone imports from there. We can remove those in the future.