siesta icon indicating copy to clipboard operation
siesta copied to clipboard

[Obsolete, but see end of thread] Add Combine support and RxSwift extension

Open luxmentis opened this issue 5 years ago • 15 comments

Hi Paul,

Here's support for Combine and RxSwift as we talked about. Combine is in the main library code; RxSwift is in Extensions.

It's influenced somewhat by the ReactiveCocoa extension and the talk around that.

As ever, there's more than one way to skin any particular cat, and perhaps especially so in RxSwift. But I've been fairly conservative - nothing too outrageous here.

Feedback entirely welcome of course.

Adrian

luxmentis avatar May 07 '20 04:05 luxmentis

@pcantrell There's a new dependency - RxSwift - for tests. Travis is failing but I see you download deps from elsewhere, so I guess you'd need to update that. The Github action is also failing - doesn't seem to have included the extensions file during build, so unresolved symbols. What magic has to happen here?

luxmentis avatar May 07 '20 04:05 luxmentis

Really excited to take a good look at this! I'm utterly consumed with helping my students with their final projects for another week or so, so please bear the delay in getting to it — but I'm excited to see it when I do.

pcantrell avatar May 07 '20 05:05 pcantrell

No problem. I'm in the process of making RxSwift and Combine copies of the Github browser example, which I'm quite enjoying. I'll add these alongside the existing example. The code I've already committed shouldn't change though, so review without fear if you get to it before I add the examples (which sounds unlikely).

luxmentis avatar May 07 '20 10:05 luxmentis

Sounds fun. Are you using SwiftUI for that?

Also, possibly relevant: I'm considering a refactoring that extracts a Resource.State struct that contains both data, error, and loading status as a single unit, adding some type safety in the mix, and possibly even deprecating the existing state accessors. It seems like that could/should be a unifying basis for reactive publishers that could then add publishers for more specific properties as a convenience.

pcantrell avatar May 07 '20 14:05 pcantrell

I haven't gone down the SwiftUI path yet. I should get onto that though (in general, not for this project) – I'm starting to feel laggardly rather than prudent.

So there'll be a lot in common between the three versions of the example project. Enough that I'll look at having a single project with multiple VC variants and targets rather than create a whole lot of duplication to maintain.

Yes, I've created a ResourceState (really should be Resource.State) struct with typed content (https://github.com/bustoutsolutions/siesta/pull/311/files#diff-29b03fc19444b88093f250673f1623fd), so it'll be easy enough to switch to the official version once it's ready. Seems sensible to do that before merging if the timing is right.

One thing I did find is that I wanted ResourceState to contain a latestEvent: ResourceEvent too. There are times I think when you want an event rather than just state, e.g. if you're generating analytics for errors, or if you're using alerts for failures rather than an overlay. And if I thought of those, no doubt there'll be other cases too. What do you think?

luxmentis avatar May 07 '20 23:05 luxmentis

OK, I've added reactive variants to the example project and have made an effort to avoid too much duplication. Same project, separate targets, shared storyboard and API, variants of the main VCs.

After saying I wouldn't, I made some changes to the main code too.

luxmentis avatar May 11 '20 01:05 luxmentis

Hi @pcantrell - I've just published a couple of blog posts about the combination of Siesta and Combine/Rx. They're pointing at my fork for now. https://luxmentis.org/swift-reactive-heaven-with-siesta-and-combine/

luxmentis avatar Jun 10 '20 00:06 luxmentis

This is really exciting work, Adrian!

I have to warn you: the Minneapolis neighborhood that you've seen on the news, the epicenter of this global uprising, is my neighborhood. I live right near the 3rd precinct building that burned down. My head has been almost entirely outside of Work Universe, much less Open Source Work Universe, since the police murdered George Floyd. I am only just now getting back into the regular rhythms, and have quite a backlog.

Realistically, I'm going to be very slow to respond to this. Please don't take that slowness as a lack of interest!

pcantrell avatar Jun 10 '20 01:06 pcantrell

Oh man, I hadn't made that connection. Yes of course - whenever you can.

It must be difficult to be optimistic with the general political climate over there, but I hope you get to make some of the necessary changes despite lack of leadership from the top.

luxmentis avatar Jun 10 '20 01:06 luxmentis

Hey Paul! I hope you're doing well.

Could you give an update on the status of this PR? I am considering using Combine with Siesta, but I'd feel much better about putting it in a production app if it was reviewed and in master.

P.S. You never let me know how I could give you an open source bonus to celebrate my app being converted to fully Siesta-based! DM me to let me know.

jordanpwood avatar Sep 04 '20 13:09 jordanpwood

Hey @pcantrell, looking for an update on this... Are there plans for this to be merged? What work needs to get this into the main branch?

jaspermayone avatar Nov 19 '22 15:11 jaspermayone

Pull requester here. Obviously this is a bit out of date by now, but if @pcantrell is interested I'm still keen to update it and get it merged or discuss implementation etc. I continue to use it in real-world code.

luxmentis avatar Nov 20 '22 03:11 luxmentis

Pull requester here. Obviously this is a bit out of date by now, but if @pcantrell is interested I'm still keen to update it and get it merged or discuss implementation etc. I continue to use it in real-world code.

As do we! I have sent an email to burstoutsolutions, as they seem to not be working on code projects anymore.

jaspermayone avatar Nov 21 '22 01:11 jaspermayone

I will keep you posted @luxmentis, and thank you for getting back to me!

jaspermayone avatar Nov 21 '22 01:11 jaspermayone

Hey @jdogcoder @jordanpwood @pcantrell and anyone else interested in using SIesta with Combine and SwiftUI,

Short version is, this PR is now obsolete, but my new SiestaExt project does all the Combine stuff, plus it adds some nice SwiftUI support and other goodies.

Here's a SwiftUI taster to get you interested – multiple resources, plus a loading spinner, error display and a retry button. Note the brevity! (This, combined with the GithubAPI class, is complete working code.)

struct MultipleSampleView: View {
    let repoName: String
    let owner: String

    var body: some View {
        VStack(alignment: .leading, spacing: 10) {
            Text("\(owner)/\(repoName)")
            .font(.title)

            ResourceView(
                GitHubAPI.repository(ownedBy: owner, named: repoName),
                GitHubAPI.activeRepositories,
                statusDisplay: .standard
            ) { (repo: Repository, active: [Repository]) in
                if let starCount = repo.starCount {
                    Text("★ \(starCount)")
                }
                if let desc = repo.description {
                    Text(desc)
                }

                Text("In unrelated news, the first active repository is called \(active.first!.name).")
            }

            Spacer()
        }
        .padding()
    }
}

Keen to hear any feedback so please chime in.

Bit sad this original PR didn't go anywhere, but I'm still a fan of Siesta even after all this time, hence the new library. I've archived my Siesta fork, but the code is still visible so any apps using it will still work (including a bunch of things I've written).

I'm leaving this PR open for the moment so any interested parties can see this news.

Although there's technically no reason this sort of code needs to be part of Siesta itself, ultimately (if Siesta has a future) it would be worth at least considering integration into the main repo because discoverability. I shouldn't think anyone's going to think to go looking for a Siesta extension project.

As ever, I think Siesta is an underrated piece of work that deserves more attention from the community (and also some more maintenance!).

luxmentis avatar Feb 27 '24 03:02 luxmentis