-
Notifications
You must be signed in to change notification settings - Fork 14
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
Synchronize creation of Events and Authors #632
Conversation
I found a Core Data threading issue while running the unit tests with the Nos Diagnostics scheme. I'll get it fixed and update the PR. |
I fixed the threading issues but now the unit tests, specifically the SocialGrapheCache tests are taking forever to execute 🤔 |
It turns out using an in-memory persistent store was causing the SocialGraphTests to take a long time. I couldn't figure out why so I switched to using an on-disk store for those. This is now ready for review. |
This fixes #446 as well as some problems @martindsq was having with the
stories/read
branch. It's a different manifestation of the issue described in #385 where we were creating two identical Event records in different threads. In the past we fixed these by ensuring that the two duplicate events were merged into one when the context was saved. This results in one Event on disk, but the two Events in memory would still have differentNSManagedObjectIDs
until reloaded from the store. This breaks SwiftUI's observation code, meaning that if you were viewing a note or profile for the first time SwiftUI might be observing an objectID that never gets updated even after we download the relevant data from the network.I fixed it by moving all Event and Author creation onto a single context. It was hard to do this elegantly without needing to pass two contexts (one for creation and one for processing, editing, etc.) all around the app. In the end I used @dependency injection to get the creation context into the creation functions, which meant I needed to change them from
class
to instance functions. It's a little janky, but I think this is much preferable to the confusion and possible developer errors it would cause to be passing two contexts around.