-
Notifications
You must be signed in to change notification settings - Fork 3
Add iOS Preload List #250
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
Add iOS Preload List #250
Conversation
| * @todo Provide this data from the host app | ||
| */ | ||
| const preloadData = { | ||
| const defaultPreloadData = { |
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.
@dcalhoun I retained this here for the bundled editor and other offline use – I don't love having more than one source of truth, but it seemed like a pragmatic way to do it (especially in the absence of an Android implementation). We could move this to JSON and load it from Android and iOS in a single code path later, if needed?
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.
Retaining this for now makes sense.
I like the idea of relocating it to a JSON file when we add an Android implementation. It seems ideal to have the native code always provide this value—either the fetched site data or the fallback value.
| /// This struct is serialized to JSON and injected into the WebView as `window.GBKit`, | ||
| /// providing the JavaScript code with all the information it needs to initialize | ||
| /// the editor and communicate with the WordPress REST API. | ||
| public struct GBKitGlobal: Sendable, Codable { |
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 moved this into native code because I really wanted it under test – doing everything with strings left me with a bunch of hard-to-debug failures where I was passing something invalid to the JS side and trying to figure out the problem using the web debugger wasn't serving me well – it's a pretty straightforward object so I don't think it's a bad trade, but interested in y'alls thoughts.
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.
Makes sense.
The number of times we pass/reassign these same values feels a little ceremonial, but some of it may be unavoidable and I'm sure there are situations where having GBKitGlobal will make unexpected outcomes less cryptic.
|
|
||
| // Originally found at https://forums.swift.org/t/support-embedding-json-like-objects-in-swift-code/38329/13 | ||
|
|
||
| public enum JSON: Sendable, Equatable, Hashable, CustomStringConvertible { |
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.
We're getting a bunch of the data from the server as JSON, which we pass into the editor. The trouble is, if the server is misconfigured and sends non-JSON, it'll crash the editor. I'd rather get a validation error instead, so this object exists to give something slightly nicer and more strongly-typed than JSONSerialization to work with.
4e96ed4 to
930207d
Compare
930207d to
7fb256b
Compare
| /// Enables logging of all network requests/responses to the native host | ||
| public let enableNetworkLogging: Bool | ||
| /// Don't make HTTP requests | ||
| public let isOfflineModeEnabled: Bool |
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 name is a bit misleading and the documentation acknowledges that. It's not for offline mode but for scenarios when you open an editor without a site. In that scenario, which we might have for GBK-based comments, the user is still required to provide siteURL and all the other required parameters.
I would suggest to:
- Remove
isOfflineModeEnabledfor now since there is currently no need for it - Refactor
EditorConfigurationlater and extract WP-site related field into a dedicated struct. The property with this type will be optional. If it'snil, that would be the indication to "load the editor without attempting to connect to a WP site".
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.
Remove isOfflineModeEnabled for now since there is currently no need for it
If I do this, it'll break the bundled editor in the demo app
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.
Refactor EditorConfiguration later and extract WP-site related field into a dedicated struct. The property with this type will be optional. If it's nil, that would be the indication to "load the editor without attempting to connect to a WP site".
I strongly agree with this as a future improvement.
| The preloading system consists of several interconnected components: | ||
|
|
||
| ``` | ||
| +---------------------------------------------------------------------+ |
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.
(nit) I would suggest to avoid committing documentation as it can be generated on the fly. The committed docs will likely get outdated quickly.
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.
Can't say I agree here – I don't think someone should have to say "hey bot, please generate some docs for me" if they're trying to understand how the system works?
Any bots working on the codebase can/should also reference this to know how it's supposed to work.
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 believe there is value in the documentation. I think the key is finding the right level of detail; the more detailed the static documentation, the more likely it will become outdated (or pose an unwieldy maintenance burden).
We might consider:
- Reducing the number of examples; relying upon a single example showcasing the high-level concept of utilizing the disparate parts together.
- Limit the times we document low-level implementation details—e.g., header filtering.
| ) | ||
|
|
||
| self.cache.storeCachedResponse(response, for: URLRequest(url: url, method: httpMethod)) | ||
| Thread.sleep(forTimeInterval: 0.05) // Hack to make `URLCache` work |
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.
Can you elaborate what this is for? It shouldn't be required. Is it only for unit tests?
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 an unused DiskKeyValueCache class and EditorKeyValueCache protocol in this PR. Would it make sense to use these if URLCache doesn't provide the required guarantees? If no, the unused code needs to be removed. These classes are also currently public, which it looks like they should not be.
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 tests fail without it, yeah. It seems like whatever thread safety is provided by the class comes at the cost of eventual consistency. If there were a callback when the data is actually committed that'd help a lot. I'd rather have a tiny delay in async functions but have guarantees that when the function returns I can immediately fetch what I just stored, so this isn't the worst trade-off.
I'd considered using the KeyValueCache objects as a replacement, but I figured if we'll adopt them we should probably do so wholesale across the library? I don't really want to do that as part of this PR so WDYT about just dropping them for now and we can revisit this question later?
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 dropped EditorKeyValueCache in c8d4f11
| NavigationStack(path: $navigation.path) { | ||
| AppRootView() | ||
| .navigationDestination(for: RunnableEditor.self) { editor in | ||
| EditorView(configuration: editor.configuration, dependencies: editor.dependencies) |
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.
(nit) ideally, you'd want to show these as modals to match how it's used in the apps
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.
Addressed in 50800e0.
| /// A protocol for making authenticated HTTP requests to the WordPress REST API. | ||
| public protocol EditorHTTPClientProtocol: Sendable { | ||
| func GET(url: URL) async throws -> (Data, HTTPURLResponse) | ||
| func OPTIONS(url: URL) async throws -> (Data, HTTPURLResponse) |
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.
(just checking) Is OPTIONS supposed to be used to fetch settings?
(nit) I'd suggest to provide a single perform(request: URLRequest) as part of the protocol (or `perform(_ method:url:) as an alternative). It's trivial to construct a request with an HTTP method you want. The naming (all caps) doesn't look like idiomatic Swift.
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 OPTIONS supposed to be used to fetch settings
Yep – it wants the settings schema, not the settings themselves
| private func perform(request: URLRequest) async throws -> (Data, HTTPURLResponse) { | ||
| var signedRequest = request | ||
| signedRequest.setValue(self.authHeader, forHTTPHeaderField: "Authorization") | ||
| signedRequest.timeoutInterval = 60 |
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.
(nit) 60 is the default timeout interval.
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.
Addressed in 71f68fa
|
|
||
| func log(_ level: EditorLogLevel, _ message: @autoclosure () -> String) { | ||
|
|
||
| public func log(_ level: EditorLogLevel, _ message: @autoclosure () -> String) { |
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 presume the logger should be internal.
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.
Yep! Addressed in 6aa25bc
|
|
||
| // If we don't have dependencies yet, we need to load them | ||
| if case .start = viewState { | ||
| self.viewState = .loading(self.loadEditorTask) |
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.
(nit) it' a bit confusing that accessing a property triggers a request
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.
WDYT about 5cdbc5b?
|
|
||
| if isWarmupMode { | ||
| startEditorSetup() | ||
| self.loadEditorWithoutDependencies() |
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'm not sure a separate code path is needed. In the previous implementation, it would simply load the editor which will in turn prefetch any of the assets it needs.
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.
Noted about this elsewhere, but the prefetching is inverted and this is just for pre-compiling JS now.
| private var loadEditorTask: Task<Void, Never> { | ||
| Task(priority: .userInitiated) { | ||
| do { | ||
| let dependencies = try await self.editorService.prepare { @MainActor progress 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.
I'm not completely sure I understand the new refresh logic, but it seems that it didn't retain the behavior from the previous system.
- Coalescing . Previously, the editor would start fetching the resources in
warmupusing a sharedEditorService(per site). If it's the first launch and the user opens the editor during prefetching, the editor subscribes to the same task. If the user opens the editor quickly after the refresh finishes, it would not performs the requests again. - Refresh. In the previous implementation,
warmupwould refresh the dependencies in the background. I can't seem to find where it refreshes the data. It appears that once the data is cached, the editor users cached data indefinitely.
There may be a couple more things.
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 host app should have control over how the editor loads. In a low-battery or cellular connection situation, for-instance it might be preferable to just re-use what's on-device already. Reducing the coupling between loading and displaying the editor allows a lot of app-side changes without modifying the library. It also allows atomic bundles – if a download is in progress we don't need to wait for it and can just display the editor immediately (or the host app can decide to wait for the download to finish). Lastly, splitting this out means that the editor can load entirely offline (whereas that doesn't currently work in trunk).
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.
So, the refresh logic is not in the scope of the PR, it will not be part of GBK, and it will be implemented separately in the host apps, correct?
the coupling between loading and displaying the editor allows a lot of app-side changes without modifying the library
I would still consider keeping as much of this logic in GBK to make it easier to iterate on it and provide the app with only the minimum required public API. Currently, it just needs to let the editor know when it needs to refresh the data, which is handled by the warmup method. The app doesn't need to know anything else, so there is minimum coupling between the host app and the framework.
if a download is in progress we don't need to wait for it and can just display the editor immediately
I believe this was also the case in the previous implementation. It would write a new manifest on disk for the editor to use only when all assets are loaded. If manifest changes, it would only fetch the assets that have changed. The editor would use the previous manifest until the new one is fully ready. I think it's more clear with explicit bundles, but it also requires a bit more management of files and directories.
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.
So, the refresh logic is not in the scope of the PR, it will not be part of GBK, and it will be implemented separately in the host apps, correct?
Yes and no – everything the apps need for refresh is in EditorService. You can check out how SitePreparationView.prepareEditor / SitePreparationView.prepareEditorFromScratch work. There are basically four scenarios an app can choose:
- The app does nothing to prepare the editor, so the editor loads its own dependencies while displaying a progress bar. Those dependencies are cached and reused indefinitely. (I can see a good argument that in this case there should be a non-indefinite caching policy).
- The app prepares the editor with default params, which reuses the local cache.
- The app prepares the editor by making an
EditorServicewithcachePolicy: .maxAge(seconds:), setting some value that makes sense for its needs. - The app prepares the editor by making an
EditorServicewithcachePolicy: .ignore, which reloads everything.
The editor would use the previous manifest until the new one is fully ready.
I couldn't find any guarantee of this – it seems actuallyRefresh is gated behind Task.sleep(for: .seconds(7)), but I couldn't find anything that would prevent dependencies(for configuration from running at the same time (particularly on a slow internet connection if the user left one post and reloaded another while using a shared EditorService).
I think it's more clear with explicit bundles, but it also requires a bit more management of files and directories.
Yeah, there's more path management, though I liked that cleanup is very straightforward and fast. Trade-offs, I guess🤷
64a6df6 to
71f68fa
Compare
71f68fa to
c8d4f11
Compare
dcalhoun
left a comment
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 implementation tested well for me. I never encountered a situation where the editor failed to load, and it loaded quickly during my testing on my iPhone SE.
I noticed a couple of things while testing.
First, there appears to be a retain cycle causing the WebView to never be released. We addressed a similar issue before in WP-iOS.
Second, I note there is an unexpected margin or overflow scroll occurring atop the editor. It only seems to occur for the default editor, though.
| /// This struct is serialized to JSON and injected into the WebView as `window.GBKit`, | ||
| /// providing the JavaScript code with all the information it needs to initialize | ||
| /// the editor and communicate with the WordPress REST API. | ||
| public struct GBKitGlobal: Sendable, Codable { |
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.
Makes sense.
The number of times we pass/reassign these same values feels a little ceremonial, but some of it may be unavoidable and I'm sure there are situations where having GBKitGlobal will make unexpected outcomes less cryptic.
| * @todo Provide this data from the host app | ||
| */ | ||
| const preloadData = { | ||
| const defaultPreloadData = { |
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.
Retaining this for now makes sense.
I like the idea of relocating it to a JSON file when we add an Android implementation. It seems ideal to have the native code always provide this value—either the fetched site data or the fallback value.
|
@dcalhoun, it won't let me reply inline, so just noting this one:
I saw this as well – I saw the note in the code about |
Co-authored-by: David Calhoun <github@davidcalhoun.me>
Co-authored-by: David Calhoun <github@davidcalhoun.me>
I second this suggestion. The changes do feel far more complex than the previous implementation. Some of that complexity may be necessary—we are implementing far more capability—but there may also be opportunity to reduce complexity through fewer abstractions (if unnecessary or lower-value ones exist).
This describes my experience as well. I did my best to comprehend and test the changes, but the quantity of changes makes it challenging to achieve a high level of confidence. |
This stuff is all site-specific – you can't set it globally. |
The editor uses this internally to render the progress bar, so I'm not sure this makes sense.
Unless you plan to disable all paths to the editor until this preloading is complete, there's no way to guarantee this. Finding a way to message that the editor isn't ready yet is substantially more complicated than the editor showing a loading screen.
We're not trying to entertain the user, we're trying to help them understand what's going on. After 10s of loading, a "cute" message becomes infuriating – the user just wants to know what's going on. |
Co-authored-by: David Calhoun <github@davidcalhoun.me>
Co-authored-by: David Calhoun <github@davidcalhoun.me>
Co-authored-by: Alex Grebenyuk <grebenyuk.alexander@gmail.com>
fa1346b to
9fba044
Compare
Done in 354d8f9, but I retained the code flow explanations because it's asynchronous between the VC and JS, and I'd like someone to be able to follow along without having to ask an LLM to explain it. |
Done in 935fd8a |
There's also no straightforward way to JSON-serialize |
There's no good one-size-fits-all solution here. For WP.com sites with Jetpack blocks, we should probably fetch on a regular basis because that code changes frequently. For self-hosted sites on lousy hosting, it probably makes sense to use The (opinionated) default is
Yes, it's useful for testing but that's not all it's there for. |
Restore the Swift Observation framework pattern that was likely unintentionally removed in PR #250, allowing the code editor toggle to properly trigger `updateUIViewController` when the state changes.
Restore the Swift Observation framework pattern that was likely unintentionally removed in PR #250, allowing the code editor toggle to properly trigger `updateUIViewController` when the state changes.


What?
Adds a preloading system that pre-fetches WordPress REST API responses and caches editor assets before the editor loads. By injecting cached data directly into the JavaScript runtime, the Gutenberg editor can initialize without waiting for network requests.
Why?
Editor loading performance isn't great when connecting to WP sites because Gutenberg needs a lot of API data. This fixes that issue (and simplifies how the editor loads – if we want to suggest the user install plugins, for instance, we should prepare the data ahead of time and inject it as part of
EditorDependencies. If we want to pre-load aWKWebViewto further speed up loading, it should go inEditorDependencies. A single loading path makes things very straightforward.How?
New GutenbergKitLogic Module
Preloading Infrastructure
EditorService- Top-level object that coordinates fetching dependencies.EditorDependencies- Container for everything the editor needs to load.EditorPreloadList- Bundles pre-fetched API responses (post types, themes, settings) for injection into JavaScriptEditorURLCache- Disk-based response caching with configurable TTL policiesSee the docs for an outline of how it all works.
Demo App Enhancements
SitePreparationViewscreen for testing preloading behaviorTesting Instructions
I've added a lot of logic tests.
I recommend testing with the demo app on a real device. I used the following test sites:
Here's some interesting test cases:
EditorDependenciesobject. It's almost instant, but not quite fast enough to avoid detection. We could look at further improving this, but injectingEditorDependenciesfully resolves it so IMHO that should be the path forward.EditorDependenciesfrom the HTTP cache), then tap "Prepare Editor ignoring cache". While that's in-progress, hit "Start Editor". The editor will load properly even though the download didn't finish because asset bundles are atomic – the next time you launch the editor, the one that was downloading will be used instead.Review Suggestions
I'd suggest going commit-by-commit. The tests aren't terribly interesting, I'd suggest reading the names – they do what they say they do. The
GutenbergKitLogicshould be extremely boring – lots of small pieces that compose together. TheEditorService,RESTAPIRepository, andEditorHTTPClientare probably the most interesting. I'd suggest mostly focusing on the adoption inGutenbergKit– how the logic is used to deliver the behaviour we want. I movedEditorViewControllerto a very simple state machine because we have multiple paths through that code.