Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

luxmentis
Copy link

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
Copy link
Author

@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?

@pcantrell
Copy link
Member

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.

@luxmentis
Copy link
Author

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).

@pcantrell
Copy link
Member

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.

@luxmentis
Copy link
Author

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
Copy link
Author

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
Copy link
Author

luxmentis commented Jun 10, 2020

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/

@pcantrell
Copy link
Member

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!

@luxmentis
Copy link
Author

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.

@jordanpwood
Copy link

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.

@jaspermayone
Copy link

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?

@luxmentis
Copy link
Author

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.

@jaspermayone
Copy link

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
Copy link

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

@luxmentis
Copy link
Author

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 luxmentis changed the title Add Combine support and RxSwift extension [Obsolete, but see end of thread] Add Combine support and RxSwift extension Feb 27, 2024
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.

None yet

5 participants