-
Notifications
You must be signed in to change notification settings - Fork 3
Add async Swift Pagination #382
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
Conversation
cae5ecc to
ff8e42b
Compare
3753ee3 to
11a7334
Compare
7bf8014 to
adf6d80
Compare
7ce1005 to
865129d
Compare
| public init(params: ResponseType.ParamsType, transform: @escaping Transformer) { | ||
| self.params = params | ||
| self.transform = transform | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better to not make this initializer (and the Transformer) public, since it makes sense that only this library can create PaginationSequence instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in d597868
| publisher.send(completion: .failure(error)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that the publisher doesn't emit elements unless fetch is called doesn't follow combine's programming model.
I don't think we really need this combine library/target, because it doesn't really relate to WordPressAPI. Its core is more like a generic solution of wrapping AsyncSequence in Publisher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agreed – we'll need some support for this on the Rust side, so I've removed it for now in ba6ba90
3c4aa13 to
1900424
Compare
1900424 to
d597868
Compare
| } | ||
|
|
||
| public struct PaginationSequence<ResponseType: PaginatableResponse>: AsyncSequence { | ||
| public typealias Transformer = (ResponseType.ParamsType) async throws -> ResponseType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this public too?
| } | ||
| } | ||
|
|
||
| struct ListViewSequence: AsyncSequence { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this struct implementation equavilant of asyncSequence.compactMap({ ($0 as? [ListViewDataConvertable]).asListViewData() })?
Builds on #380 by adding
Combine andAsyncSequence-based pagination.You can try it out by running the sample app and loading posts for https://content-heavy.wpmt.co – you'll see the posts slowly load in as they're paginated (the code has the page size set to 5 so it's easier to see).
I put theCombinestuff in its own module so that it doesn't have to be loaded (for instance, it'd be annoying for Linux environments).Depends on #386 to build properly on Linux because Swift 6 brings unifiedFoundation.