Refactor Sample App and Logging #202
Conversation
|
|
||
| /// Manages persistence of remote editor configurations | ||
| class ConfigurationStorage { | ||
| class ConfigurationStorage: ObservableObject { |
There was a problem hiding this comment.
This class is now just injected using EnvironmentObject which removes some code.
| let apiRoot = try await client.apiRoot.get().data | ||
|
|
||
| let canUsePlugins = apiRoot.hasRoute(route: "/wpcom/v2/editor-assets") | ||
| let canUseEditorStyles = apiRoot.hasRoute(route: "/wp-block-editor/v1/settings") |
There was a problem hiding this comment.
We can now hit the actual site and figure out if it supports plugins or editor styles, which makes the demo app more useful for testing with real sites.
jkmassel
force-pushed
the
refactor/sample-app-and-logging
branch
from
763def5 to
9ecd147
Compare
There was a problem hiding this comment.
Nice!
It was hard to debug issues early in the lifecycle before I could attach the Web Inspector.
In addition to logging, which is must have, it would be fantastic to also add an option to show a "Continue" button in EditorViewController to allow you to debug the start up sequence using Safari Developer Tools:
EditorViewControllercreatesWKWebView- You connect it to it in Safari and tap "Continue"
- Editor gets loaded
- You can see everything that's being loaded and use the Safari performance tools – profit
| let log = try message.decode(EditorJSMessage.LogMessage.self) | ||
|
|
||
| if log.level >= controller.configuration.logLevel { | ||
| print(log.message) |
There was a problem hiding this comment.
(suggestion/future)
Let's send it to the delegate (GutenbergEditorControllerDelegate) and let the app use its logging infrastructure so we could log critical errors and performance metics in production so if someones reaches out with a performance issue, we had an idea of what's going on.
There was a problem hiding this comment.
Does it make sense to instead rely upon the existing conditional logging in JavaScript?
The new native editor configuration could set the level in JavaScript. Doing so would avoid unnecessary (currently conditionally ignored here on line 308) bridge events. It would also reduce the conditionals to a single place: the JavaScript logger implementation.
I don't have strong feelings on this. Sharing it for consideration.
There was a problem hiding this comment.
I've addressed this in 8bc0040 – the logging config is injected into the GBKit global and applied to the editor at that point.
| locale: String, | ||
| isNativeInserterEnabled: Bool, | ||
| editorAssetsEndpoint: URL? = nil, | ||
| logLevel: LogLevel = .warn |
There was a problem hiding this comment.
I would disable logging by default so there is no impact on performance and enable it only in the demo and in the app when the app needs it.
There was a problem hiding this comment.
There's no real concept of "disabling" logging in the library at the moment, and IMHO errors always be emitted. I've set the default to error for now.
If it becomes a problem we can change it later but that'd be a bigger refactor.
ios/Demo-iOS/Gutenberg.xcodeproj/xcshareddata/xcschemes/Gutenberg.xcscheme
Outdated
Show resolved
Hide resolved
ios/Demo-iOS/Gutenberg.xcodeproj/xcshareddata/xcschemes/Gutenberg.xcscheme
Outdated
Show resolved
Hide resolved
| let log = try message.decode(EditorJSMessage.LogMessage.self) | ||
|
|
||
| if log.level >= controller.configuration.logLevel { | ||
| print(log.message) |
There was a problem hiding this comment.
Does it make sense to instead rely upon the existing conditional logging in JavaScript?
The new native editor configuration could set the level in JavaScript. Doing so would avoid unnecessary (currently conditionally ignored here on line 308) bridge events. It would also reduce the conditionals to a single place: the JavaScript logger implementation.
I don't have strong feelings on this. Sharing it for consideration.
* Refactor sample app to split views * Raise logger messages up to the app level * Address logging suggestions * Address suggestions * Revert port change * fix linter issues * Fix module issue
What?
While doing performance improvements, I ran into a few issues:
This PR solves both to avoid rebasing later.
Why?
See above
How?
I split the view hierarchy into a couple of files and removed a bunch of code.
I added a logging bridge message and added it to the editor configuration.
Testing Instructions
If you build and run the app there should be no difference. If you update the remote configuration builder in the demo app to use
.debugas the log level you'll see some messages from existing usage.Accessibility Testing Instructions
n/a