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

Add EventStream.fromPublisher #114

Closed
wants to merge 6 commits into from

Conversation

armanbilge
Copy link
Contributor

@armanbilge armanbilge commented Nov 16, 2023

And effectively implementing (part of) the Reactive Streams spec.

In practice, this makes it possible to convert an fs2.Stream to an EventStream via the Flow.Publisher interface. Monix and ZIO also interop with Reactive Streams, although this is not cross-published for Scala.js yet.

Edit: It turns out Monix also supports Reactive Streams on Scala.js!

@armanbilge armanbilge requested a review from raquo as a code owner November 16, 2023 02:17
@raquo
Copy link
Owner

raquo commented Nov 16, 2023

Looks great, thank you! I should be able to include this in the next version, I just need to write some docs on how to use it with FS2.

@armanbilge
Copy link
Contributor Author

I just need to write some docs on how to use it with FS2.

I've been thinking about this. After chatting with @BalmungSan we realized that we need to add one more API on the FS2 side. Interop is possible with the current APIs but I think it would require flatMapping EventStreams which is sub-optimal. We want to expose a new "unsafe" API that gives direct access to the Publisher from the fs2.Stream without wrapping it in Resource or IO.

@raquo
Copy link
Owner

raquo commented Nov 21, 2023

Hm, I guess I'll hold off merging until that's cleared up. I'm not sure why flatMap would be necessary on the airstream side, but if you'll fix that need eventually, that's moot.

@armanbilge
Copy link
Contributor Author

I'm not sure why flatMap would be necessary on the airstream side

Is there a way to get from a Future[Publisher[A]] to an EventStream[A] using fromFlowPublisher with the current signature and not using flatMap?

@raquo
Copy link
Owner

raquo commented Nov 21, 2023

Is there a way to get from a Future[Publisher[A]] to an EventStream[A] using fromFlowPublisher with the current signature and not using flatMap?

No.

To clarify, unnecessary flatMap-s in Airstream are bad because they create unnecessary transactions (which can cause FRP glitches), but this particular flatMap, although unnecessary, does not increase the potential of FRP glitches, I think: since the events originate outside of Airstream (in FS2), Airstream will emit them in separate transactions anyway, even without flatMap.

Eliminating the need for a flatMap is still beneficial though: less boilerplate, and better efficiency.

@armanbilge
Copy link
Contributor Author

Hm, I guess I'll hold off merging until that's cleared up.

FWIW I don't think it is necessary to hold this PR. The API here is solid, and interop is possible today. We can document as-is, and update the docs once FS2 adds the new API, without requiring a new Airstream release.

@armanbilge
Copy link
Contributor Author

armanbilge commented Nov 25, 2023

After typelevel/fs2#3346 is released, interop will be as simple as

import cats.effect.unsafe.implicits._ // implicit IORuntime
EventStream.fromPublisher(fs2Stream.unsafeToPublisher())

@armanbilge armanbilge changed the title Add EventStream.fromFlowPublisher Add EventStream.fromPublisher Nov 25, 2023
@armanbilge
Copy link
Contributor Author

FS2 v3.9.4 was released with the new interop APIs.

@raquo
Copy link
Owner

raquo commented Jan 21, 2024

@armanbilge Great, thanks, and for the docs too, I'll merge this PR for the next 17.x release

@raquo raquo mentioned this pull request Feb 26, 2024
raquo added a commit that referenced this pull request Feb 26, 2024
Arman's implementation from #114, rebased on master, moved into a separate package, etc.
raquo added a commit that referenced this pull request Feb 26, 2024
Arman's implementation from #114, plus some of my bikeshedding

---------

Co-authored-by: Arman Bilge <[email protected]>
@raquo
Copy link
Owner

raquo commented Feb 26, 2024

@armanbilge Merged now, thanks again!

I wanted to rebase this on latest master, add a minor commit to this, and squash it all, but I couldn't figure out how to do that while preserving this PR and without bothering yourself for such menial work, so I've done all that in #122. The resulting commit is here. No API changes, just moved things around a bit.

@raquo raquo closed this Feb 26, 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.

3 participants