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 syntax to convert Future to Rx #42

Merged
merged 1 commit into from
Aug 1, 2017
Merged

Conversation

OlivierBlanvillain
Copy link
Owner

No description provided.

Copy link
Collaborator

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

The implementation is so small and it requires no 3rd party dependencies, what do you think about including it in the core module?

import scala.util.Try
import mhtml.{Rx, Var}

object Utils {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The module implementation is almost smaller than the build changes! :D

@OlivierBlanvillain
Copy link
Owner Author

@olafurpg That's would create an asymmetry with a potential sub project for monix.Task. I clearly don't want a monix dep on the main project, but I also don't want to make it more convenient to use scala.concurrent.Future over monix.Task since I strongly believe the latter is superior.

@olafurpg
Copy link
Collaborator

The point of the toRx helper is to get rid of scala.concurrent.Future, the most common use case being to wrap Ajax.{get,post} requests like here:

.fromFuture(Ajax.get(s"$api/$suffix"))

Is there a similar http client like Ajax.get based on monix?

@bbarker
Copy link
Contributor

bbarker commented Mar 23, 2017 via email

@olafurpg
Copy link
Collaborator

RosHttp is not free from scala.concurrent.Future https://github.com/hmil/RosHTTP/search?utf8=%E2%9C%93&q=scala.concurrent.Future&type= My point is just that monix and concurrent.Future are not mutally exclusive. I don't see the need for a -future specific module if you can't avoid scala.concurrent.Future in the first place. If there exists a scala.concurrent.Future free http client, then I'm 👍 for creating a -future module.

@OlivierBlanvillain
Copy link
Owner Author

Ajax.{get,post} is library based, the fact that it's published by default with scala-js-dom is not a sufficient argument to give a privileged status to scala.concurrent.Future.

Also a port to monix Task should be trivial: https://gist.github.com/OlivierBlanvillain/4105d05aea258b56f9317b3c81072118

Copy link
Collaborator

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Sweet, then I'm fine with creating a -future module 👍

@bbarker
Copy link
Contributor

bbarker commented Jul 26, 2017

"I also don't want to make it more convenient to use scala.concurrent.Future over monix.Task since I strongly believe the latter is superior."

Future seems to be more frequent in occurrence than monix, partly because it is standard (not because it is better). I feel like this comes up frequently enough that it is worth merging. A FAQ or gist could be linked to if we want to make it easy for people to add something similar when using monix, and if nothing else, the Future example provides a good template for people to start from when implementing conversions from other async libraries.

@OlivierBlanvillain
Copy link
Owner Author

Alright, I'm sold, I'm going to get back to this PR and merge something for scala.concurrent.Future

@OlivierBlanvillain OlivierBlanvillain changed the title Add monadic-rx-future project Add syntax to convert Future to Rx Aug 1, 2017
@OlivierBlanvillain OlivierBlanvillain merged commit aacc5f3 into master Aug 1, 2017
@OlivierBlanvillain OlivierBlanvillain deleted the monadic-rx-future branch August 1, 2017 11:22
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