-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Update how block editor settings are fetches and stored #24510
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
|
||
/// - warning: Decoding can take a non-trivial amount of time. | ||
func getBlockEditorSettings() -> [String: Any]? { | ||
guard let data = rawBlockEditorSettings?.data else { |
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 changes ensures that when Blog
entity is loaded, the settings blob it not loaded in memory. It is achieved by using a relationship (see BlobEntity
) , which is a standard way to approach it. The entity is also configured to store large blobs in external storage, which is more efficient than putting it in a database.
In the previous implementation, the blob was stored in the options
dictionary, which is designed for tiny key-value entities and can no be used for large blobs for performance reasons.
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 benefits of the new blob store make a lot of sense.
/// Refreshes the editor settings in the background. | ||
func refreshSettings() { | ||
Task { @MainActor in | ||
try? await fetchSettings() |
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 new refreshSettings
method does not decode the editor settings. It used to happen every time you open the "My Site" screen and it was relatively expensive, and was happening on the main thread. We don't need to read the current settings to send a request to refresh them.
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.
Avoiding unnecessary decoding is smart.
} | ||
isRefreshing = false | ||
private func fetchSettings() async throws -> [String: Any] { | ||
if let task = refreshTask { |
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.
This is a standard pattern how you approach this in Swift Concurrency. Task
works as a future and can have multiple subscribers.
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.
I see now how proper utilization of Task
simplifies the implementation.
@@ -24,48 +25,50 @@ class RawBlockEditorSettingsService { | |||
guard let dictionary = response as? [String: Any] else { | |||
throw NSError(domain: "RawBlockEditorSettingsService", code: 1, userInfo: [NSLocalizedDescriptionKey: "Invalid response format"]) | |||
} | |||
blog.rawBlockEditorSettings = dictionary | |||
let objectID = TaggedManagedObjectID(blog) | |||
try? await ContextManager.shared.performAndSave { context in |
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 previous implementation wasn't performing a save. It now saves and does it in the background.
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 that to say the original storage referenced below was only ever stored in memory and not actually persisted elsewhere?
In the previous implementation, the blob was stored in the
options
dictionary, which is designed for tiny key-value entities and can no be used for large blobs for performance reasons.
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 previous implementation updates blog.rawBlockEditorSettings
in memory in an NSManagedObject
entity on the main context (main thread), but it never explicitly persists on disk. That's not to say that it never gest persisted. There are other areas of the app that occasionally call try mainContext.save()
to their save the in-memory changes on disk. So, it eventually gets saved together with other changes.
The standard concurrency model for working work with Core Data is to save on a background context and only read on main (this is why it's called viewContext in NSPersistentContainer
). Unfortunately, we are not strictly following this model.
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.
Thank you for explaining further. 🙇🏻♂️
2a4c34d
to
f362632
Compare
|
App Name | WordPress | |
Configuration | Release-Alpha | |
Build Number | 27549 | |
Version | PR #24510 | |
Bundle ID | org.wordpress.alpha | |
Commit | b381193 | |
Installation URL | 4eld1j6bhftt8 |
|
App Name | Jetpack | |
Configuration | Release-Alpha | |
Build Number | 27549 | |
Version | PR #24510 | |
Bundle ID | com.jetpack.alpha | |
Commit | b381193 | |
Installation URL | 2f04gr8a4lcv8 |
c38886e
to
c179e9e
Compare
private static var services: [TaggedManagedObjectID<Blog>: RawBlockEditorSettingsService] = [:] | ||
|
||
@MainActor | ||
static func getService(forBlog blog: Blog) -> RawBlockEditorSettingsService { |
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 previous implementation with isRefreshing
was ineffective because there were multiple instances of RawBlockEditorSettingsService
: one for My Site and one for each editor.
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.
That makes sense now that you call attention to it. I like the new design with static methods for retrieving the individual services.
|
@@ -182,6 +185,7 @@ | |||
<relationship name="postTypes" optional="YES" toMany="YES" deletionRule="Cascade" destinationEntity="PostType" inverseName="blog" inverseEntity="PostType" syncable="YES"/> | |||
<relationship name="publicizeInfo" optional="YES" maxCount="1" deletionRule="Cascade" destinationEntity="PublicizeInfo" inverseName="blog" inverseEntity="PublicizeInfo"/> | |||
<relationship name="quickStartTours" optional="YES" toMany="YES" deletionRule="Cascade" destinationEntity="QuickStartTourState" inverseName="blog" inverseEntity="QuickStartTourState" syncable="YES"/> | |||
<relationship name="rawBlockEditorSettings" optional="YES" maxCount="1" deletionRule="Cascade" destinationEntity="BlobEntity" syncable="YES"/> |
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.
Note the "cascade" deletion rule, which is what you need.
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.
Thank you for noting this.
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.
This tested well for me.
I greatly appreciate you taking the time to improve this and explain various aspects of the implementation. Educational for me and a more robust implementation.
|
||
/// - warning: Decoding can take a non-trivial amount of time. | ||
func getBlockEditorSettings() -> [String: Any]? { | ||
guard let data = rawBlockEditorSettings?.data else { |
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 benefits of the new blob store make a lot of sense.
private static var services: [TaggedManagedObjectID<Blog>: RawBlockEditorSettingsService] = [:] | ||
|
||
@MainActor | ||
static func getService(forBlog blog: Blog) -> RawBlockEditorSettingsService { |
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.
That makes sense now that you call attention to it. I like the new design with static methods for retrieving the individual services.
@@ -24,48 +25,50 @@ class RawBlockEditorSettingsService { | |||
guard let dictionary = response as? [String: Any] else { | |||
throw NSError(domain: "RawBlockEditorSettingsService", code: 1, userInfo: [NSLocalizedDescriptionKey: "Invalid response format"]) | |||
} | |||
blog.rawBlockEditorSettings = dictionary | |||
let objectID = TaggedManagedObjectID(blog) | |||
try? await ContextManager.shared.performAndSave { context in |
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 that to say the original storage referenced below was only ever stored in memory and not actually persisted elsewhere?
In the previous implementation, the blob was stored in the
options
dictionary, which is designed for tiny key-value entities and can no be used for large blobs for performance reasons.
/// Refreshes the editor settings in the background. | ||
func refreshSettings() { | ||
Task { @MainActor in | ||
try? await fetchSettings() |
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.
Avoiding unnecessary decoding is smart.
} | ||
isRefreshing = false | ||
private func fetchSettings() async throws -> [String: Any] { | ||
if let task = refreshTask { |
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.
I see now how proper utilization of Task
simplifies the implementation.
@@ -182,6 +185,7 @@ | |||
<relationship name="postTypes" optional="YES" toMany="YES" deletionRule="Cascade" destinationEntity="PostType" inverseName="blog" inverseEntity="PostType" syncable="YES"/> | |||
<relationship name="publicizeInfo" optional="YES" maxCount="1" deletionRule="Cascade" destinationEntity="PublicizeInfo" inverseName="blog" inverseEntity="PublicizeInfo"/> | |||
<relationship name="quickStartTours" optional="YES" toMany="YES" deletionRule="Cascade" destinationEntity="QuickStartTourState" inverseName="blog" inverseEntity="QuickStartTourState" syncable="YES"/> | |||
<relationship name="rawBlockEditorSettings" optional="YES" maxCount="1" deletionRule="Cascade" destinationEntity="BlobEntity" syncable="YES"/> |
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.
Thank you for noting this.
Here the test scenario with fresh install and timeout in the editor set to 300 seconds:
Screen.Recording.2025-04-29.at.3.24.28.PM.mov