Skip to content

Conversation

jkmassel
Copy link
Contributor

What?

Moves the Swiftsoup dependency out into its own package that can be conditionally used.

Why?

The library current requires that its hosting app adopt Swiftsoup. It's only used for a small corner of the functionality where regex might not properly parse the data we get back from the server.

How?

Creates a new AssetManifestParser package that can be included (or not). By default, the implementation now uses a regex-based approach, but warns that it's not recommended.

Testing Instructions

I've added unit tests for both the default and new parsers that cover the parsing better than it was before.

I'd recommend testing it against a site using the assets plugin.

Accessibility Testing Instructions

n/a

Screenshots or screencast

n/a

@jkmassel jkmassel requested a review from dcalhoun September 10, 2025 23:26
@jkmassel jkmassel force-pushed the decouple/swift-soup branch 2 times, most recently from 832d686 to 142b02e Compare September 11, 2025 00:30
@jkmassel jkmassel force-pushed the decouple/swift-soup branch 4 times, most recently from cda24fb to b64d3ab Compare September 11, 2025 00:52
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent and changes seem sound from review. Unfortunately, the implementation does not work from my testing. The cached assets are never used when inspecting network activity of the WebView. Do you experience this as well?

I tested by configuring the Demo app. I could not test with WP-iOS due to the current incompatibility with WP-iOS with the merge of #146.

Trunk Changes
Image Image

@@ -112,7 +119,7 @@ public struct EditorConfiguration {
return String(data: jsonData, encoding: .utf8) ?? "undefined"
}

public static let `default` = EditorConfigurationBuilder().build()
public static var `default` = EditorConfigurationBuilder().build()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change from let to var?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants