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

Version 0.10.0 - Static Language Support #304

Conversation

benjchristensen
Copy link
Member

Manual merge of pull #300 from @mattrjacobs

This will make RxJava completely static by removing all Object overloads (see #208 and #204).

I'm submitting this before it being 100% ready so people can review and provide feedback.

Open items to append to this pull request before merging:

1) subscribe with map is not handled yet

The following signature needs to be made static. Right now the lack of this combined with removal of Functions.from dynamic language functionality has broken this.

public Subscription subscribe(final Map<String, Object> callbacks)

2) Core artifact naming convention

Should rxjava-core-x.y.x.jar become rxjava-x.y.z.jar since the concept of core+language no longer applies?

I think I'd prefer this:

  • rxjava-x.y.z.jar
  • rxjava-groovy-x.y.z.jar
  • rxjava-clojure-x.y.z.jar
  • rxjava-scala-x.y.z.jar
  • rxjava-jruby-x.y.z.jar
  • rxjava-kotlin-x.y.z.jar
  • rxjava-dynamic-x.y.z.jar (object overload for any language)
  • rxjava-groovy-clojure-x.y.z.jar (multi-language jar)

Only one of those jars is needed hence the reason why I think the 'core' term is no longer needed as it communicated the fact it was always needed.

Any contrib modules would be: rxjava-contrib-module-name-x.y.x.jar

3) Dependencies from languages to core still exist

The build still will result in Maven Central POM files requires rxjava-core from the language version despite that not being the case. Need to eliminate this dependency in the artifact.


Implementation notes originally posted at #204 (comment):

After implementing and throwing away a few different approaches we have landed on a solution we feel will balance the various competing priorities.

Our goals are:

  • support static typing for Java/Scala/Kotlin etc by removing the Object overloads
  • support any JVM language, static or dynamically typed
  • allow all languages to use the same rx.Observable class so that we don't divide libraries with helpers such as GroovyObservable, ClojureObservable etc that then need to be converted back and forth when doing interop
  • do not require special classloaders or agents to enable runtime bytecode generation
  • do not remove static operators to enable proxying
  • small jars and limited or no dependencies

The solution we have arrived at will work as follows:

  • The rxjava-core source code will delete all Object overload methods and be pure static java.
    • Any language that supports functional interfaces directly (such as Java 8 and XTend) can use the Java core version directly.
  • Languages needing specific lambda/clojure type mapping to the Func_/Action_ types will have language specific Jars created via build-time bytecode generation.
    • Any method with a Func_/Action_ argument will be overloaded with a version supporting the language requirements.

For example:

The default Java version:

public static <T> Observable<T> filter(Observable<T> that, Func1<T, Boolean> predicate)

A Groovy version:

public static <T> Observable<T> filter(Observable<T> that, groovy.lang.Closure predicate)
  • A jar per language will be created as follows:
    • rxjava-x.y.z.jar
    • rxjava-groovy-x.y.z.jar
    • rxjava-clojure-x.y.z.jar
    • rxjava-scala-x.y.z.jar
    • rxjava-jruby-x.y.z.jar
    • rxjava-kotlin-x.y.z.jar

A project will include just the jar that meets their language needs, there will no longer be a "core" jar plus the language adaptor.

The drawback of this is that mixing two of these in a classpath will result in non-deterministic loading (whichever is loaded last wins) and that is the version that will be used. This means if a library depends on rxjava.jar but is using Groovy and needs rxjava-groovy.jar it is up to the developer of that project to make sure they have only the rxjava-groovy.jar version. This is not ideal but is a one-time pain setting up a build and is better than the constant pain of missing static typing or converting to/from different Observable implementations for different languages.

  • At this time we are optimizing for projects using a single language or Java + another language. If there are use cases where people are trying to mix multiple languages in a very polyglot manner we have two options:
    • include an rxjava-dynamic.jar version that re-adds the Object overloads
    • include build configs for common combinations of languages such as rxjava-groovy-clojure.jar
  • Language adaptations (such as clojure which has preferred idioms that necessitate wrapping) will still be possible through the language-adaptor projects and be included in the appropriate language jars.

This should not break any code but will require a slight change to the build dependencies in your project when we release this.

We hope that this enables the RxJava project to better achieve its goal of being polyglot and targeting the JVM in general and not any specific languages without sacrificing interop or idiomatic usage in each of the languages.

mattrjacobs and others added 14 commits July 3, 2013 23:11
…age's native function support

* Only hooked up to rxjava-groovy in this commit
* Adds an Object overload to Observable methods via code generation
…to version_0.10.0_multi-static

Conflicts:
	rxjava-core/src/main/java/rx/Observable.java
I couldn't get these to work once we removed the Object overloads that was apparently being invoked instead of the staticly typed method so removing them.
If someone else can get these generics working then please submit a new pull request.
These methods will need different names as they have the same argument count so break with dynamic overloads once we do static handling of dynamic languages.
- needed for Scala build to succeed
@cloudbees-pull-request-builder

RxJava-pull-requests #174 SUCCESS
This pull request looks good

@mattrjacobs
Copy link
Contributor

  1. I don't think I follow your point about making the subscribe(Map<String, Object>) method static. Wouldn't that break all existing scripts that use this method? Could we add a subscribe(Map<String, Closure>) or subscribe(Map<String, IFn>) variant in the dynamic JARs? That would also suggest tightening up the core signature to subscribe(Map<String, Function>).
  2. This naming convention makes sense to me. I will change rxjava-core to rxjava in the artifact name, at least. I think I will leave the directory rxjava-core alone, since it makes more sense there, and IMO, it would be confusing to have a dir like RxJava/rxjava/... I don't plan on bringing rxjava-kotlin along with this pull request, but I can review Kotlin Language Adaptor #292 to help fit that into the 0.10 world.
  3. I think I understand this, though I'm already at the edge of my Gradle ability. If I'm understanding this correctly, we should not change the build steps for a project - we just need to change the generated Maven POM. That makes sense - I'll figure out a way to accomplish that.

@jmhofer
Copy link
Contributor

jmhofer commented Jul 6, 2013

Looks good, all in all. I'll try this out with my Scala sample as soon as possible. Thanks for the awesome work!

@benjchristensen
Copy link
Member Author

Thanks @jmhofer for stepping in to test out these changes.

@mattrjacobs Answers to your questions ...

  1. subscribe(Map)

If we leave the Map<String, Object> then the Functions.from capability needs to keep working.

The other option is we change that to be Map<String, Function> as you suggest and then auto-generate the versions for each language.

  1. Thanks, and agreed on the directory name, but that can change if we want in the future without impacting the artifact name once you set the name.

  2. Maven POM

I think the simplest solution is just not have any "compile" dependencies, only "provided". I believe your Gradle scripts just need to adjust to include provided dependencies, not just compile runtime dependencies and then it will work. I would not try messing with POM files directly. If something is specified as "provided" then it is used for compilation but not included in the POM when pushed to Maven Central.

@cloudbees-pull-request-builder

RxJava-pull-requests #175 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #176 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

RxJava-pull-requests #177 SUCCESS
This pull request looks good

@jmhofer
Copy link
Contributor

jmhofer commented Jul 8, 2013

Currently, if you publish RxJava to your local Maven repo, you'll find that the rxjava-scala and rxjava-swing POMs include a dependency on "rxjava-core", which doesn't exist anymore as a module, as it was renamed to "rxjava".

Unfortunately, I don't know enough about Gradle to be able to fix this. My guess would be that it's not enough to set the baseName of the JAR to just "rxjava". There should be a way to tell gradle that the module is called "rxjava" even though the directory is called "rxjava-core".

@jmhofer
Copy link
Contributor

jmhofer commented Jul 8, 2013

Here's another problem: Excluding "*$UnitTest" from JARs that might be included by Scala will cause the Scala compiler to crash with a Typer error because it expects defined inner classes to exist. This means that at least in rxjava-swing, the exclude can't be done.

@jmhofer
Copy link
Contributor

jmhofer commented Jul 8, 2013

I've created a mini-PR against the unit test problem here: benjchristensen#1

@jmhofer
Copy link
Contributor

jmhofer commented Jul 8, 2013

I got my little sample (https://github.com/jmhofer/rxjava-samples) working with this PR. The code itself was very easy to adapt. The implicits seem to be working fine.

One small thing I noticed: The very long names of the implicits are a bit distracting, imho. Should I want to use one of these functions explicitly, I'd prefer shorter names like simply func1 instead of scalaFunction1ToRxFunc1, for example.

@mattrjacobs
Copy link
Contributor

Responses to @jmhofer:

  1. POM references to rxjava-core: In the Scala case, I prefer including all of the core Rx classes directly in the rxjava-scala.jar, so that there is no dependency at all. This avoids the problem entirely. For rxjava-swing, I'll work on the Gradle side to make the reference correct. This might take the form of changing the reference or actually changing the name of rxjava-core to rxjava - we'll see how friendly Gradle is.
  2. Excluding inner classes: I've also come across the Scala compiler error on Classfile parsing with excluded inner classes. At the moment, unit tests exist in rxjava-core.jar and rxjava-scala.jar only. Going forward:
    • rxjava-core: These unit tests only exist so that rxjava-scala can be built. I will exclude them, add a new unfilteredJar Gradle task, and build rxjava-scala off of unfilteredJar.
    • rxjava-scala: I will continue to include UnitTests, since it's pretty likely that Scala projects are consuming this
    • rxjava-swing: I hadn't considered the case of Scala consuming this, but you're obviously right. For now, I'll err on the side of caution and unilaterally add the UnitTests back to the jar. At some later point, we might want to re-examine (rxjava-swing-scala.jar/rxjava-swing-filtered.jar). But adding them back is the safe thing to do. Your PR is exactly what I will do, thanks for that.
  3. Implicit names: In my experience, it's rare that I call implicit conversions by name, but I can see your point. My only concern is ambiguous namings. Would changing 'scalaFunction1ToRxFunc1' to 'toRxFunc1' (and other similar naming changes) be satisfying?

@jmhofer
Copy link
Contributor

jmhofer commented Jul 9, 2013

@mattrjacobs: All your suggestions seem sensible to me.

Concerning implicit naming: It may be relatively rare, however using the implicits often leads to your having to specify the parameter types of the closures being implicitely converted, which can be more verbose and confusing than making the conversion explicit and then being able to shorten the closure syntax.

Names like toRxFunc1 are ok with me, not too long and still unambiguous, sounds like a good compromise.

@cloudbees-pull-request-builder

RxJava-pull-requests #181 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #182 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #183 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

RxJava-pull-requests #188 FAILURE
Looks like there's a problem with this pull request

@benjchristensen
Copy link
Member Author

Replaced by #319

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.

4 participants