Implement a new site plugins list

[crazytonyli]

Note

This PR is built on top of #24014.

The new screen follows the pattern established in the self-hosted site User Management. An application password is required to access this feature (when it's turned on).

Here is a recording of this screen:

plugins.mp4

As part of this PR, I made a small change to CachedAsyncImage. It now accepts an interim object (ImageURLResolver), which eventually produces an image URL. The icons in the plugin list is not part of the plugins response. Their url has to be loaded separately.

I could spin up separate tasks to load them and then refresh the plugins list after they are loaded. But I thought making them part of the image loading process might be more straightforward in terms of managing the loading tasks: SwiftUI would take care of them.

Of course, that comes with a downside: All URLs are put into an async call, which is definitely slower than just using them directly in the calling thread. But I thought that's an okay compromise to make.

Regression Notes

1. Potential unintended areas of impact

2. What I did to test those areas of impact (or what existing automated tests I relied on)

3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• I have considered adding accessibility improvements for my changes.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

• WordPress.com sites and self-hosted Jetpack sites.
• Portrait and landscape orientations.
• Light and dark modes.
• Fonts: Larger, smaller and bold text.
• High contrast.
• VoiceOver.
• Languages with large words or with letters/accents not frequently used in English.
• Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
• iPhone and iPad.
• Multi-tasking: Split view and Slide over. (iPad)

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr24023-4d62c18
	Version	25.7
	Bundle ID	org.wordpress.alpha
	Commit	4d62c18
	App Center Build	WPiOS - One-Offs #11441

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr24023-4d62c18
	Version	25.7
	Bundle ID	com.jetpack.alpha
	Commit	4d62c18
	App Center Build	jetpack-installable-builds #10473

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[kean]

I hit the following assertion when using a self-hosted site with a Jetpack plugin:

wpAssert(blog.account == nil, "The Blog argument should be a self-hosted site")

[kean]

I think it's OK to show a description, but I would limit it to 2 lines. It looks off when there are many lines with this tiny font.

The first two lines might help someone identify what the plugin is about and if they want to "Read More", they'll tap to open the details. It's not unlike how the app renders excepts for posts.

[crazytonyli]

This part is likely to change as I'm developing the details screen. Do you mind using this design as it is for this PR, and I'll revisit it in follow up PRs?

[kean]

I wouldn't risk using this on iOS on data that's coming from the web. It's slow, and it might hang or crash. For a short description like this, let's use plain text, and use a full-blown web view in the details screen.

[crazytonyli]

I don't think this description text would have long and complex html. It's mostly decorative, like bold or italic text. Using plain text would show things like <strong> in the content.

[kean]

Using plain text would show things like <strong> in the content.

It will need to be sanitized with tags removed. There is an existing method to do so which is used in a few places. I can't remember the exact name now.

[kean]

It seems like a bit of an over-kill to add a protocol for one use-case. I'd suggest using composition:

struct PluginIconView: View {
    let slug: String
    @State var iconURL: URL?

    var body: some View {
        ZStack {
            if let iconURL {
                CachedAsyncImage(url: iconURL)
                /// ...
            } else {
                /// loading state
            }
        }
        .task {
            iconURL = try? await resolveIconURL(for: slug)
        }
    }
}

It's less code, and it won't require adding complexity to CachedAsyncImage.

[crazytonyli]

Oh nice. I'll use your suggested approach instead.

[kean]

I would hold off adding DataStore or other generic protocols until there are more use cases and it becomes clearer what it should look like.

It's hard to beat Core Data. It's starting to look a lot like WordPressFlux in terms of the amount of protocols, generics, and indirection, which is probably not a great sign.

In this case, it seems the app just needs to open a plugin list, fetch them, and put them in memory for the list to display. I'd suggest putting this in memory in a view model for now:

struct PluginListViewModel {
     @Published private(set) var plugins: [Plugin] = []

    // use the service directly here to fetch the list 
}

I'd also strongly recommend avoiding making everything an individual actor as it will become extremely hard to reason about very quickly.

[kean]

The best part about Core Data is that it allows you to easily implement data-driven UI by subscribing to data directly.

[kean]

Btw, please feel free to ignore this in the scope of this PR, but I think it needs a bit more consideration. There is a bit too much ceremony for this simple scenario of showing a static list.

[crazytonyli]

I would hold off adding DataStore or other generic protocols until there are more use cases and it becomes clearer what it should look like.

We are working on it iteratively at the moment. I think it'd be hard to come up with a flexible approach upfront.

It's hard to beat Core Data.
[...]
The best part about Core Data is that it allows you to easily implement data-driven UI by subscribing to data directly.

I agree that the data-driven UI part is nice. However, I think the app does need an alternative approach to presenting things on the UI. Using Core Data means we'll need to follow the established ContextManager paradigm (*), adding an entity to the same data model, saving their records to the same database file on disk, using rigid APIs to prevent data corruption, migrating the database when the data model changes, etc. I think we touched on this before when we agreed that some features do not need to be stored in the Core Data database, like reader posts. It'd be nice to use the data-drive UI feature without the other constraints.

@jkmassel also mentioned having a cross-platform data store solution in Rust. We haven't discussed it in detail, though. That'd be alternative to Core Data, which can be implemented as a DataStore here.

(*) Unless we develop another system using a different Core Data persistent container. Having two systems may be confusing.

It's starting to look a lot like WordPressFlux in terms of the amount of protocols, generics, and indirection, which is probably not a great sign.

I don't think there are too many protocols/geneircs here? It's all most one class to one protocol at the moment. protocol ServiceProtocol for class PluginService, protocol DataStore for PluginDataStore.

By "indirection", do you mean the AsyncStream part? I would not consider that indirection though, because you can see how data updates lead to UI updates by following the code path. I'd consider callback closures and notification observers indirection, because it's hard to find when they are called. But I don't think that's the case here?

[kean]

saving their records to the same database file on disk

Low-key, Core Data is an object-management system first and a database second. It can be used with an in-memory store. I wouldn't necessarily advocate for that, but you get a lot by using it. Most importantly – unique objects, observations with predicates, and first-class SwiftUI integration.

Having two systems may be confusing.

That is what I would also like to avoid because the app also already has WordPressFlux, which serves the same goals as in-memory Core Data. I personally do not know how to use WordPressFlux and it's hard to debug, and it's a bit of an issue because the app's most popular features – Stats – uses it. It can grow in complexity quickly and it's extremely hard to get right. I personally do not know any good examples where these abstractions don't turn into massive pits of complexity and edge-cases. There is definitely a lot of value in sticking to a single approach across the app regardless of how good/bad. I don't think we can realistically replace Core Data.

By "indirection", do you mean the AsyncStream part?

AsyncStream is probably not the best way to model paging because it doesn't support retries, so it seems like a no-go just for that reason. If there was an underlying type that did and that had a convenience AsyncStream wrappers for the few scenarios where it's needed, that would've been ideal. It's generally best when there are low-level APIs that are as primitive and powerful as possible.

Showing plugins is one of the simplest scenarios, and it required quite a lot of different moving parts. The main indirection – going back to the data-driven part – is that the View needs to listen to updates from a ViewModel, that in turns listens to updates from PluginService, that forwards the request to the underlying InMemoryInstalledPluginDataStore, that has a query enum with a single case. And it required closures, Combine, and AsyncStream. I think there are opportunities to keep it simpler.

[kean]

Nit-pick, this part is also a bit ambiguous:

for await update in await self.service.streamInstalledPlugins() {
             switch update {
             case let .success(plugins):
                 self.plugins = plugins

The method says "stream" and it would usually indicate that there is paging/streaming involved where it would return plugins in batches: [1, 2, 3], [4, 5, 6]. But, based on what it does, it doesn't seem to be the case – it's a simple observable property. In that case it should have a signature like: var installedPlugins: AsyncStream<[Plugin]>. I'm not sure it's an establishes practice, but that's what Apple does in TipKit. I don't know if there are more examples like this.

I think the only framework that got it right was ReactiveSwift where you had a Property type that was both observable, and you could get the current value, and you could initialize it with an observable sequence that it would automatically unwrap. @Published is close enough to this, but Combine is kind of soft-deprecated. AsyncStream is neither here nor there.

[crazytonyli]

I don't think we can realistically replace Core Data.

It depends on what you mean by "replace". IMHO, two main features are enough for our app (and probably most apps): 1) data-driven UI. 2) storing data as if they are all cached content (the source of truth lives in the server) without the need to maintain data model schema and migrate the database.

That is what I would also like to avoid because the app also already has WordPressFlux. [...] I personally do not know how to use WordPressFlux and it's hard to debug

Same here. The event flow there is not very easy to follow. I don't think there is any interest in continuing to use WordPressFlux.

AsyncStream is probably not the best way to model paging

The AsyncStream is not used for paging. The programming modal is very similar to how the app uses Core Data.

In a typical list, you have a UITableView, which displays results yielded by NSFetchedResultController. In the background, there would be HTTP requests to fetch data from the server. The data is stored in Core Data as HTTP responses come in, which leads to UI updates where you see the list keeps growing.

Here, similar to NSFetchedResultController, we have listStream: AsyncStream. It observes data store changes and yields a full result of a given query. The result is then displayed on the SwiftUI list.

It's generally best when there are low-level APIs that are as primitive and powerful as possible.

Totally agree. Similar to composition (with primitive tools) over inheritance (of a pre-defined structure). Maybe this is a better API with a nicer syntax sugar. Maybe we can implement something similar to @Environment(\.objectContext) ..., @FetchRequest .... IMO, the most important thing here is an alternative to Core Data (again, I don't mean as a one-to-one replacement). If the rigid structure of "View Mode <-> Service <-> Data Store" is not it, maybe we can evolve it into something better.

And it required closures, Combine, and AsyncStream. I think there are opportunities to keep it simpler.

These different things are used in their isolated area. For example, AyncStream is used for SwiftUI updates, because SwiftUI works pretty nicely with async things. Combine is used in the in-memory data store to emit updates, because that's the most convenient ad-hoc way to convert manual updates to an AsyncStream. If we have another Core Data backed data store, we'll probably use NSFetchedResultController or NSNotification, instead of Combine. If we have another SQLite backed data store, it'll probably use some sqlite hook to observe database changes, instead of Combine.

In that case it should have a signature like: var installedPlugins: AsyncStream<[Plugin]>.

Just want to double check, do you mean simply changing the function to a computed property?

[kean]

It depends on what you mean by "replace".

I mainly mean removing it across the board in the app and replacing it with something else. In terms of features, at some point, the new system will have to have almost everything that Core Data does. There are, of course, screens where it's not needed. It's not used in Stats, for example, but this screen is a simple function of converting static JSON to views. It doesn't really need abstractions to work as you can simply fetch data, preprocess it, and put in @Published properties in View Models or maybe even directly in views and you are done.

continuing to use WordPressFlux

I think it's worth giving it a benefit of a doubt, learning how it work, and seeing if it can be fixed. In many ways, it seems like a system with exactly the same goals as DataStore, but it shows it end state with the features and the complexity already there.

changing the function to a computed property?

It can be a method, but I suggest removing the name "stream". Apple didn't make anyone favors by naming AsyncStream the way they did. It's a simply convenience type for implementing AsyncSequence – AnonymousAsyncSequence or something like it.

I don't know if it's only me, but when I see "stream", I think this – a stream of bytes. Open a stream, read bytes in chunks, close it. It has little to no relation with observable sequences.

[crazytonyli]

I mainly mean removing it across the board in the app and replacing it with something else. In terms of features, at some point, the new system will have to have almost everything that Core Data does.

I think it should be okay if this app, or any app, uses something else instead of Core Data? Speaking from my own experience, this codebase is my first app that uses Core Data.

By the way, just in case I may have miscommunicated, I don't think we'd want to remove Core Data entirely. I think we want an alternative, at least, that can be used for things that do not need to be in Core Data. The "in-memory data store" is not the alternative solution. It's merely a placeholder for the alternative solution.

It doesn't really need abstractions to work as you can simply fetch data, preprocess it, and put in @published properties in View Models or maybe even directly in views and you are done.

That's the gist of most viewing screens, isn't it? Users, plugins, themes, media even? I agree that's a lot of ceremonies. But we typically break the code down into different types, instead of putting them all into view and/or view model, for better separations of concerns, testability, readability, and all that, which would bring in the side effect of having those ceremonies.

As I mentioned before, the data lives in memory for now, but it will be saved to disk later. When that time comes, hopefully, the change will simply be to replace the "in-memory data store" with a "SQLite data store" or something.

Another thing I want to mention is that the point of abstractions is extracting common themes out of these JSON-displaying screens. We'd want something reusable to replace this code (which may need to be repeated on different screens). Instead of calling updateDisplayingPlugin (see the pseudo code below) imperatively, the view models' performQuery functions respond to data changes.

naive pseudo code

class ViewModel {
    @Published var displaying: [Plugin]

    var search: String {
        didSet {
            self.updateDisplayingPlugin()
        }
    }

    var filter: PluginFliter /* all, active, inactive, etc */ {
        didSet {
            self.updateDisplayingPlugin()
        }
    }

    var fetchedPlugins: [Plugin] {
        didSet {
            self.updateDisplayingPlugin()
        }
    }

    func fetchPlugins() async {
        self.fetchedPlugins = restApi.listPlugins()
    }

    func updateDisplayingPlugin() {
        if search.isEmpty {
            self.displaying = fetchedPlugins.filter { by filter }
            return
        } else {
            self.displaying = fetchedPlugins.filter { by search }
        }
    }
}

It can be a method, but I suggest removing the name "stream". [...] I don't know if it's only me, but when I see "stream", I think this – a stream of bytes.

I get what you mean now. I'll rename this function.