Simplify User Service #23818
Simplify User Service #23818
Conversation
Generated by 🚫 Danger |
jkmassel
force-pushed
the
simplify/user-service
branch
from
27a79c5 to
14f860c
Compare
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23818-14f860c | |
| Version | 25.4.2 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 14f860c | |
| App Center Build | jetpack-installable-builds #10103 |
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23818-14f860c | |
| Version | 25.4.2 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 14f860c | |
| App Center Build | WPiOS - One-Offs #11063 |
| [Cacheable](objects.values) | ||
| } | ||
|
|
||
| func invalidate(_ id: Cacheable.ID) { |
There was a problem hiding this comment.
If it removes an object, let's call it remove.
| func invalidate(_ id: Cacheable.ID) | ||
| } | ||
|
|
||
| actor InMemoryCache<Cacheable: Identifiable & Sendable>: ObjectCache { |
There was a problem hiding this comment.
I think it should ultimately be modeled closely to how NSManagedObjectContext works: data-driven and with a single main-thread confined context with all entities that you can read/write synchronously and you can easily observe.
It's generally not a good idea to introduce actors because you end up with more concurrency than you need, making it hard to understand and debug what's going on and in which order. For the UI layer, I'd use @MainActor. For a model layer, it's generally best to synchronize all subsystems on a single global actor to eliminate as much concurrency as possible.
|
|
||
| for try await page in await self.client.api | ||
| .users | ||
| .sequenceWithEditContext(params: UserListParams(perPage: 5)) |
There was a problem hiding this comment.
I'm not sure AsyncThrowingStream is the right API to model paging because if it errors out, the stream terminates. There is no way to restart fetching for a given page, is there?
It may be convenient to use AsyncThrowingStream in the scenario where you want to fetch everything at once – though you should probably never do that – but it can be built on top of a more primitive API like func fetchPage(withID:) async throws -> [Element], likely even in a nice generalized way.
| func fetchPaginatedUsers() -> AsyncThrowingStream<[DisplayUser], Error> { | ||
| AsyncThrowingStream { continuation in | ||
| let handle = Task { | ||
| let all = await self.cache.all() |
There was a problem hiding this comment.
I believe there is a retain cycle here: self -> tasks -> handle: Task -> self.
| self.users = users | ||
| return users | ||
| } | ||
| func fetchPaginatedUsers() -> AsyncThrowingStream<[DisplayUser], Error> { |
There was a problem hiding this comment.
If I understand this correclty, this stream's outputs are cached users (which may be empty or all the pre-fetched users), users at page 1, users at page 2, ..., END. If that's the case, how fetchPaginatedUsers is used in the View Model is wrong. The View Model should not collect all the stream's outputs into an array.
Fixes #
To test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: