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

Fix launchPagingStore #603

Closed
wants to merge 10 commits into from
Closed

Conversation

matt-ramotar
Copy link
Collaborator

@matt-ramotar matt-ramotar commented Feb 14, 2024

Closes #602

Description

The cause of #602 is we were not synchronizing updates to single items with the collection they belong to. This PR fixes that by handling the streaming and updating of single item data within a collection.

Other changes:

  • Introducing Pager, PagingData, Joiner, Streamer, and KeyFactory interfaces. I think this is cleaner, easier to reason about, easier to test.
  • Moving launchPagingStore into RealPager implementation.
  • Emitting a state flow of PagingData rather than StoreReadResponse. My thought is this will make it easier to support UI-level extensions, similar to collectLazyPagingItems.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

Test Plan

  • Unit tests

Checklist:

Before submitting your PR, please review and check all of the following:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my change is effective
  • New and existing unit tests pass locally with my changes

Additional Notes:

  • I removed the original design doc from the source code. It is still attached to the PR that introduced paging. I am working on Migrate website to Docusaurus #595 and plan to include documentation on paging internals.
  • @digitalbuddha @yigit - I'm launching a child coroutine for each single in a collection. I think this will not scale well for apps with lots of data. We should think about limiting the number of concurrent coroutines and cancellation strategies.

mramotar_dbx added 9 commits February 17, 2024 15:24
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
@matt-ramotar matt-ramotar force-pushed the matt-ramotar/launch-paging-store branch from 9bdae15 to 8ee88bb Compare February 17, 2024 20:24
@matt-ramotar matt-ramotar marked this pull request as ready for review February 17, 2024 21:00
childScope.launch {
// Locking the streams map to check and manage stream jobs.
mutexForAllStreams.withLock {
// Checking if there's no active stream for the key.
Copy link
Collaborator

Choose a reason for hiding this comment

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

just for readablility, it would be nicer to move these into helper methods.
especially, it is hard to visually track mutexForAllStreams usages without separate methods.

launchChildCollectionLocked

kind of interface might be good.

alternatively, you can use a helper method to create the coroutine (lazily) and then only start it if you can get the lock (e.g. allStrams[key] is null).

Comment on lines +94 to +96
val childJob = launch {
initStreamAndHandleSingle(single, childKey, key)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

so, i'm guessing this is for individual item updates (just double checking).

This was a request in paging, the ability to notify single item changes. But we've never implemented it and it is really hard (since item data change might change ordering). Ideally, instead of observing each child individually, it would be better to let streamer do that job. Technically, I think the streamer should be responsible to emit a new value when any elements matching the data set is changed.

Since I've not looked at store for a while, i might be widely wrong :D. But it feels like better to avoid watching every single child individually, also knowing that, room/sqldelight both will send notification when anything in the table changes, not necessarily an individual item.

Copy link
Collaborator

Choose a reason for hiding this comment

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

another option for individual element watching is is to make streamer(key) return a list of ids and fetch those ids individually (that is how firebase paging works, or worked at some time :) ).
otherwise, i think it is better for streamer to invalidate everything properly instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving the responsibility of listening for child updates from RealPager to the streamer makes sense. I also like that users will be able to decide for themselves what level of granularity in observing updates.

// Storing the main job and handling its completion.
allStreams[key] = mainJob

mainJob.invokeOnCompletion {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we shoiulda void child collectors. but if we cant, you can do this better by making their jobs a child of mainJob so that you won't need manual cleanup.

Signed-off-by: mramotar_dbx <mramotar@dropbox.com>
Comment on lines +10 to +16
var combinedItems = mutableListOf<PostData.Post>()

data.values.forEach { responseData ->
responseData.items.let { items ->
combinedItems = (combinedItems + items).distinctBy { it.postId }.toMutableList()
}
}

Choose a reason for hiding this comment

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

combinedItems's type should be List.
And toMutableList is unnecessary when using List type

@matt-ramotar matt-ramotar mentioned this pull request Mar 9, 2024
3 tasks
@matt-ramotar
Copy link
Collaborator Author

Closing. See #611

@matt-ramotar matt-ramotar deleted the matt-ramotar/launch-paging-store branch March 16, 2024 17:51
@matt-ramotar matt-ramotar mentioned this pull request Mar 16, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] State Flow From launchPagingStore Does Not Reflect Latest Writes in Mutable Store
3 participants