-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Idiomatic Scala Support #336
Comments
A few comments from my point of view (since I first looked at this project, I always wanted to use it from Scala): Now that the core is typesafe, imho we have the first workable Scala integration (via implicits). From here on, everthing depends on how much effort we want to put into this vs how convenient it will be for the users. What we already have is that the most important method names match ( There's a lot of room for improvements, though. It seems that due to Scala-Java interop, there are problems with type inference (I even ran into a Scala compiler crash somewhere), and sometimes For full Scala user convenience, we'd probably have to completely wrap |
|
IMHO, after looking at the code, using implicits convertions is not a good idea. I think you have better to start with a complete wrapper of the API, then you'll be able to apply scala idiom (basically writing a DSL) instead of having a Java API with some facilities for converting type. And that way you don't have to wait for https://issues.scala-lang.org/browse/SI-6221 ... which sounds anyway like a clumsy integration of Java API to me. |
Here is some background on why the current design was chosen rather than having each language with a separate version of Observable: The approach of having language specific packages/classes was pursued but did not work well because Rx is a composable library. It means that every time an For example ... From Java a library is exposed that has a method like this: rx.Observable getData() From Groovy a library is exposed with a method like: rx.groovy.GroovyObservable getOtherData() Then from Scala you need to wrap them again: rx.scala.ScalaObservable.from(getOtherData()) This means we have an To compose the two we would have: rx.scala.ScalaObservable.zip(rx.scala.ScalaObservable.from(
getOtherData()),
rx.scala.ScalaObservable.from(getData()),
... scala closure here ...); Now what does Should the above zip operator return If In short, for interop between languages it very quickly becomes a mess and our primary polyglot goal was that For this reason we chose the current language-adaptor model so |
Thanks for providing the design notes! I wonder though how often people tend to mix languages in their projects. Usability improvements in switching from a Java-based least common denominator to an idiomatic API might be significant. Does being polyglot overweigh these improvements? |
I'm not sure if wrapping |
At Netflix we are using Clojure, Groovy, Java and Scala and I know of apps running code from at least 3 of those 4 in the same JVM instance. I imagine it's not common in most environment for this type of diversity, but it is something we have wanted to support as seamlessly as possible. This is because we have wanted the That said, an idiomatic solution that works best for pure Scala apps is more important. If we can find a solution that can retain the use of Another piece of information to guide this ... the Rx.Net version in C# is defined by simple interfaces for The reason RxJava has In Scala however we do have extension methods (implicits), macros etc that theoretically can allow the At this point I become not so helpful as I am not skilled enough in Scala to have a valid opinion or guide the conversation much further. I do have some questions though for the Scala experts here:
I'd like to see unit tests or sample code demonstrating expected behavior and usage so that we're all working towards the same goal and know when we've achieved success.
Thank you everyone for your involvement, I really do want us to find the ideal solution for Scala and then as a secondary priority make it work well across the JVM for polyglot applications. |
Thank you for your swift feedback! It's a pleasure to help.
a) b) It shouldn't be necessary to write c) Use of mutable collections in situations like https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/src/main/scala/rx/lang/scala/RxImplicits.scala#L350 is really inconsistent with Scala's pursuit of immutability. I wonder whether it'd be possible to write such code in a functional way. d) Instead of taking a no-arg function, e) For a former C# developer like me, method names like f) I'm also not sure whether extension methods are even necessary. From what I would guess, in C# they are forced to use them, because interfaces can't define methods with implementations (at least, that was the story with g) h) No idea how this name could be stated more succinctly, but
E.g. in https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/src/main/scala/rx/lang/scala/RxImplicits.scala#L341 one can't drop type annotations for lambda parameters. I tried removing them and got a compilation error. (Btw why are you using 2.10.1, when 2.10.2 is already available for quite long? Did you have any problems with it?) That's an instance of https://issues.scala-lang.org/browse/SI-6221, which has been fixed in 2.11 and probably could be backported to 2.10. If you're in need of an immediate solution, we at EPFL have a compiler plugin for 2.10 that provides a backport. |
Excellent observations about the API @xeno-by. Currently, we are preparing exercises for the Coursera class Principles of Reactive Programming. For this course we would like to make an idiomatic Scala API. @samuelgruetter is working on that. The cross language compatibility pointed out by @benjchristensen is a reasonable concern. However, I think there is a lot of monolingual projects out there that would greatly benefit from the idiomatic Scala library. It seems to me that adding a Scala wrapper can be only a plus in those cases. For polyglot projects we could also take the Java rx.Observable as a common ground (being a return type of each operation and in API signatures). Then all of the Scala support can be added by an implicit conversion (rx.Observable => rx.ScalaObservable). As a long-shot maybe the interface for multi and mono lingual projects can be unified so that we do not repeat our selves. IMHO it is worth a try. |
Thanks for all the observations! I'll try to address some of the ones from @xeno-by. 1a), 1e) and 1g) are just aliasing problems, imho. We already have I didn't take a look at 1b) and collection conversions yet. This really shouldn't be necessary often, if at all. 1c) You're looking at test code here which tests the callback "at the end of the world". This is not really normal usage of the API. The whole RxJava codebase is refreshingly (though not completely) free of mutability, if you ask me. Also, the Rx API is actually there to allow you to handle all your events in a functional, immutable way. - In short, I don't see a problem here. 1d) is Scala-specific. By-name parameters are a great feature for methods like I'm not worried about 1f) either. 1h) I like the verbose name in this specific case. Imho, it's the same as
And the reason for 2.10.1 instead of 2.10.2 is currently just a technical problem with the build system. I, too, hope that this will be fixed soon. |
1c) Even that can be made functional if you replace the subscribe() with .toList().toBlockingObservable().single(); |
@abersnaze You're right, good point. I started a little experiment with a more idiomatic Scala wrapper. It's just humble beginnings currently, though. You can find it here. Comments and forks are welcome, of course. I started from the super-extends pull request in order to check how this whole thing feels in Scala. |
Scala 2.10.2 support was added in version 0.11.2 (https://github.com/Netflix/RxJava/releases/tag/0.11.2). |
Sorry, we don't change type inference in minor versions unless there's a critical bug. |
I completely agree if a wrapper is the only way to achieve idiomatic support. Monolingual projects should take first priority as that is the common case. |
We believe that the following solution would be best:
I've started to implement such an adapter and will post some code soon. Note that I've not (yet) addressed the following points:
|
@samuelgruetter Sounds awesome! Very much looking forward to your sharing the code. |
You can see what I'm doing here. Note that it's still very incomplete and not yet tested, but work is in progress. |
Great, thanks! I'll play around with it, too. |
@samuelgruetter All of that sounds great. I'll take a look next week and offer feedback. |
There's an interesting problem with |
Maybe there's a 2nd implicit somewhere, adding map? |
@samuelgruetter what is the error? if @jmhofer is right you should have an |
If anything about the core Java library is causing issues let me know. |
There's no 2nd implicit, but there's a second |
Just another guess, but then it's maybe due to the type parameter of |
Covariant support has been merged into master. This also changes the type used with |
I believe all of the structural changes are now merged to master. Are there any other changes that should be made before we release 0.12 so as to better support Scala integration? |
I've isolated the implicit conversion problem here. It's a problem with the Scala compiler. |
@samuelgruetter I'm pretty sure no one could make the scala compiler be able to solve an ambiguity like that one. If you where the compiler, which method you think should be choosed? You guys should seriously consider implementing a full wrapped scala version first, and keep multi-lang support as a cherry on cake. IMHO it's a mistake to do the opposite, maybe I'm wrong but I think most scala user will want to use it in a full scala project. And for the course, it would be good to have an idiomatic API. |
We have sufficient workarounds for the compiler bug. There's nothing fundamental that we need for the Scala adapter right now, but I'm currently trying to translate the dictionary autocomplete example written in C# from this Rx Hands On Lab to Scala to see what we can already do. For this, I miss the |
The 'throttle' operator was just merged to master as We do not yet have |
That's great that workarounds exist so we can move forward, I look forward to hearing how the autocomplete example works. If you can get that functioning is that strong enough evidence to move forward with it or are there still pieces of functionality lacking? How do you envision ongoing maintenance of this class? Do all new operators added to |
If I can get the autocomplete example functioning then I think that's strong enough evidence to move forward with it. We still lack operations in the wrapper, but it should be possible to add them all. This adapter will require some maintenance. New operators added to the Java observable will have to be added to the adapter as well, and updates of the documentation too, because the signatures are too different to automate this. |
Good to know regarding maintenance ... we'll need to figure out a reasonable way of handling that as operators get added. Do you have an idea of when I should expect a pull request with this new Scala Observable? Is this a breaking change to how the implicits support currently works for Scala, or does this wrapper only take effect when someone imports |
It's not a breaking change: The old |
So does it make sense to mark them deprecated since they're still usable and possibly useful for someone accessing operations that haven't made their way to this curated wrapper? Just checking. |
If an operation from
|
Btw |
Pull #376 has been released in 0.13.1 Thank you @samuelgruetter! |
With that one difference does it still require the two existing, or can |
If there is already code out there depending on |
Given that there's no loss of functionality, I think it makes the most sense to remove |
Removing duplication makes sense. Since |
@mattrjacobs Good idea. @daveray Well the goal should be that there is no such thing as "stuff not covered by the Scala wrapper"... Having |
Removing Can someone also please update the README (https://github.com/Netflix/RxJava/blob/master/language-adaptors/rxjava-scala/README.md) to show usage information to help someone get started? |
@samuelgruetter It's up to you guys. I'll just leave with this: open source projects live a lot longer than the enthusiasm of their contributors so I'm skeptical of a wrapper that assumes it will always be kept completely up-to-date with complete coverage forever. If some third party decided to create a project of custom observables beyond what's in RxJava, those would be more difficult to use, assuming users respect the implication of "internal" in the package name. I'll go back to Clojure-land now :) |
@daveray ScalaObservable has a method to get the wrapped Java Observable, so you should be able to use any method from its API. |
@martin-g Right. And if for whatever reason some poor Scala programmer finds herself in yucky Java-land calling these mehods, at least throw her a bone and make the Func implicits available in a non-internal package. |
@martin-g Correct me if I'm wrong, but dropping down to using rx.Observable still mandates passing in After thinking about it, +1 to @daveray's idea of making both the value class and implicits available (with the value class preferred). This will allow any temporary mismatch between |
+1 on this. There are other places where |
We can't make the build fail if It needs to be an async process for Scala developers to add wrapper methods/classes. Since this is open source we can't control people's schedule and require quick turnaround on adding the wrappers. Ideally that will happen when new functionality is added, but we can't make building and releasing dependent on that. Thus, I think we need to account for the fact that most things will have wrappers but very newly added functionality or fringe functionality may not and should still have a mechanism for being used. |
If you want rxjava-core to evolve independently then perhaps rxjava-scala On Mon, Sep 16, 2013 at 2:55 PM, Ben Christensen
|
Erik Meijer and I have discussed this but it doesn't feel like the right thing to do, as it would hurt the JVM ecosystem more than benefit it if the different languages forked from each other. There is no harm in a new version of RxJava being released without the Scala adaptor adding the new functionality, as that is no different than a separate RxScala project not yet supporting it on top of the RxJava core dependency. As we approach 1.0 the rapid iteration will slow and this will become less of an issue. Around that time as well the project may migrate into a different home than here. If you want rxjava-core to evolve independently then perhaps rxjava-scala |
I like @daveray's point: If some third party decided to create a Java project of custom observables beyond what's in RxJava, and we want to use this project from Scala, then we need the @benjchristensen I agree that it would be kind of crazy to make the build fail just because |
Closing this out as completed ... further progress/bugs can use new issues. Great work and thank you everyone involved on this. |
As of version 0.11.0 Scala support is provided through the use of implicits. Conversations on Twitter are bringing up other possible improvements. Let's use this issue to discuss.
The text was updated successfully, but these errors were encountered: