Fix how editor handles dependencies loading #24956
Conversation
kean
force-pushed
the
task/fix-loading-dependencies
branch
from
33356a5 to
7ad2ab0
Compare
| /// the network. | ||
| func getSettings(allowingCachedResponse: Bool = true) async throws -> Data { | ||
| // Return cached settings if available | ||
| if allowingCachedResponse, let cachedSettings = try await BlockEditorCache.shared.getBlockSettings(for: blogID) { |
There was a problem hiding this comment.
If local cache read using try Data(contentsOf: fileURL) were ever to fail, it would prevent fetchSettingsFromAPI from running. The cache lookup should not stop the request.
| } | ||
|
|
||
| self.editorLoadingTask?.cancel() | ||
| if isBeingDismissedDirectlyOrByAncestor() { |
There was a problem hiding this comment.
The easiest way to verify it works is by setting a breakpoint.
| editorContentWasUpdated() | ||
| } | ||
|
|
||
| private func fetchEditorDependencies() async -> EditorDependencies { |
There was a problem hiding this comment.
This does not actually throw any errors – removing throws.
| startEditor(dependencies: dependencies) | ||
| } | ||
|
|
||
| private func startEditor(dependencies: EditorDependencies) { |
There was a problem hiding this comment.
Moved this with no changes.
kean
changed the title
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 29576 | |
| Version | PR #24956 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | b0c6b7b | |
| Installation URL | 54ucqnr1c4ve0 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 29576 | |
| Version | PR #24956 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | b0c6b7b | |
| Installation URL | 5g0m9p2gnn9eg |
There was a problem hiding this comment.
You're no longer displaying an error if the editor fails to load dependencies. If the users requests site assets for the editor and we can't load them, we can't silently fail – we need to explain what's going on. Otherwise the editor looks broken and the user has no feedback about why.
One other thing you lose by dropping a state machine – it's far easier to deal with interactive plugin installation by having it. If a user doesn't have the plugins installed on their site to support the editor, we need to prompt them to install them. That can very quickly become difficult to follow without a more formal way to track state.
edit: for context, see: #24925
| } | ||
|
|
||
| return try Data(contentsOf: fileURL) | ||
| return try? Data(contentsOf: fileURL) |
There was a problem hiding this comment.
Can we log the error then return nil instead of dropping it entirely?
That way we'll see something in Sentry if something's wrong – there's a performance implication here because loading performance will get way worse for anyone who has a cache miss.
There was a problem hiding this comment.
Sure, I'll add wpAssert.
I'd be happy to add some kind of alert if we want it (in a separate PR?), but in the production version private func fetchEditorDependencies() async throws -> EditorDependencies {
let settings: String?
do {
settings = try await blockEditorSettingsService.getSettingsString(allowingCachedResponse: true)
} catch {
DDLogError("Failed to fetch editor settings: \(error)")
settings = nil
}
let loaded = await loadAuthenticationCookiesAsync()
return EditorDependencies(settings: settings, didLoadCookies: loaded)
}
func showEditorError(_ error: Error) {
// TODO: We should have a unified way to do this
}
I didn't know about the other PRs. It seems like a sequential process, so would it work if you make @MainActor
private func actuallyLoadEditor() async {
showActivityIndicator()
do {
let plugins = try await fetchPluginRecommendations()
if "has recommendation" {
await showRecommendedPlugin()
}
}
let dependencies = await fetchEditorDependencies()
startEditor(dependencies: dependencies)
}This way, it's clear what the steps are and in which order they are performed. I would use a state machine if it had more transitions between states, but again this does look a simple sequential process. I would lean into async/await for it. |
|
3 participants
Changes
RawBlockEditorSettingsServicenot proceeding with the request if cache read failseditorStatehappening every 1 microsecondIssue with "Revisions"
Prerequisites
Steps
Observed Results
RCA
viewWillDisappearis called not only when you close the screen, but also when it temporary gets covered by a different screen being presented as fullscreen modal or being pushed into the navigation stack.