Integrate GutenbergKit remote editor asset caching#24639
Conversation
Generated by 🚫 Danger |
| GutenbergKit.EditorViewController.warmup() | ||
| GutenbergKit.EditorViewController.warmup( | ||
| configuration: blog.flatMap(EditorConfiguration.init(blog:)) ?? .default | ||
| ) |
There was a problem hiding this comment.
We can move/copy this code to "warm up" the GBK editor after switching sites, which pre-fetches the remote editor assets.
| editorAssetsEndpoint.appendPathComponent("editor-assets") | ||
| self.editorAssetsEndpoint = editorAssetsEndpoint | ||
| } | ||
| } |
There was a problem hiding this comment.
This if statement is new. The rest are moved from the view controller initializer above.
| siteApiNamespace.append("sites/\(siteId)") | ||
| } else { | ||
| siteApiNamespace.append("sites/\(siteDomain)") | ||
| } |
There was a problem hiding this comment.
I lied. This if statement is also changed. The siteApiNamespace should not be ["sites/<site-id>", "sites/<mysite.wordpress.com>"], right? It should contain either the id or the domain?
Here is the original code, for easier review:
if isWPComSite {
if let siteId {
siteApiNamespace.append("sites/\(siteId)")
}
siteApiNamespace.append("sites/\(siteDomain)")
}There was a problem hiding this comment.
Oh wait... I see the code comment now. I'll revert this change.
There was a problem hiding this comment.
Correct, we need to retain this. Third-party blocks can utilize either namespace. So, we need both for proper matching, otherwise we may mistakenly add a duplicative namespace to a path already containing a namespace.
dcalhoun
changed the title
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
This works well from my testing. 🎉 Aside from the noted GBK release requirement, this seems good to merge.
Modules/Package.swift
Outdated
| //.package(url: "https://github.com/wordpress-mobile/GutenbergKit", from: "0.3.0"), | ||
| .package(path: "../../../GutenbergKit"), |
There was a problem hiding this comment.
Friendly note: we need to replace the local path with a new GBK release before merging this PR.
FYI I have a open wordpress-mobile/GutenbergKit#151 for documenting and automating the GBK release process.
| siteApiNamespace.append("sites/\(siteId)") | ||
| } else { | ||
| siteApiNamespace.append("sites/\(siteDomain)") | ||
| } |
There was a problem hiding this comment.
Correct, we need to retain this. Third-party blocks can utilize either namespace. So, we need both for proper matching, otherwise we may mistakenly add a duplicative namespace to a path already containing a namespace.
crazytonyli
force-pushed
the
try-gbk-remote-editor
branch
from
b702cb5 to
bd6fccd
Compare
|
|
⬆️ Rebased with one change: replacing the GBK local path ref to the 0.4.0 release tag. |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 28066 | |
| Version | PR #24639 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | bd6fccd | |
| Installation URL | 64rev0fg6oum0 |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 28066 | |
| Version | PR #24639 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | bd6fccd | |
| Installation URL | 1vbvsep3g39ug |
4 participants
Description
Close CMM-509. This PR uses wordpress-mobile/GutenbergKit#148.
To try it out locally:
Testing instructions