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

Spike: modularization #2228

Closed
2 of 3 tasks
danieldietrich opened this issue Mar 18, 2018 · 15 comments
Closed
2 of 3 tasks

Spike: modularization #2228

danieldietrich opened this issue Mar 18, 2018 · 15 comments

Comments

@danieldietrich
Copy link
Contributor

danieldietrich commented Mar 18, 2018

Yesterday I bootstrapped a new, modularised version of Vavr. It runs on Java 9 and will be the base for the upcoming 1.0.0 release.

In this issue I want to collect early feedback while tracking the migration from the previous code base Vavr 0.9.x to the next code base Vavr 1.0.0.

The Vavr 1.0.0 source can be found in the v1.0.0 branch.

Log

  • Created an initial Gradle project that consists of a first draft of Java 9 modules.
Gradle module Java 9 module exports requires transitive
vavr (root)
vavr-core io.vavr.core io.vavr.core
vavr-control io.vavr.control io.vavr.control io.vavr.core
vavr-collection io.vavr.collection io.vavr.collection io.vavr.control
vavr-collectionx io.vavr.collectionx io.vavr.collectionx io.vavr.collection
vavr-concurrent io.vavr.concurrent io.vavr.concurrent io.vavr.control
vavr-api io.vavr.api io.vavr.api io.vavr.collectionx, io.vavr.concurrent
  • After fiddling around with a failing Java8/Java9 multi-release build I fell back to a working Java8 only build. In my opinion a multi-release build is not practicable.
  • Started to draft the class hierarchy.
@danieldietrich danieldietrich added this to the vavr-1.0.0 milestone Mar 18, 2018
@danieldietrich danieldietrich changed the title VΛVΓ 1 VΛVΓ 1 (logbook / work in progress) Mar 18, 2018
@talios
Copy link

talios commented Mar 19, 2018 via email

@emmanueltouzery
Copy link
Contributor

emmanueltouzery commented Mar 19, 2018

Hmm i don't know about hiding Left and Right. It will make the apidoc cleaner, but it may become a hindrance if/when java gets pattern matching ( http://cr.openjdk.java.net/~briangoetz/amber/pattern-match.html ).

It depends how you go with methods which would fit only on Right. For instance Either could have getLeft and getRight both returning Option or getLeftOrThow and getRightOrThrow, while Left could have getLeft returning L and Right could have getRight returning R. So pattern matching would ensure safe calls.

But we could always revisit that if/when java does get this pattern matching. Hide now and add back then maybe. Currently java/vavr instanceof matching is not very idiomatic. In prelude.ts Either Either has getLeftOrElse, getLeftOrThrow, getOrElse, getOrThrow. Right has get. Left has getLeft.

@Opalo
Copy link
Contributor

Opalo commented Mar 19, 2018

I've looked around a bit and it seems that internal is something that makes sense in java 9 world. Whether it's internal or any other name doesn't make difference - it's a (sub)package that isn't exported via module-info.java.

@talios mentioned quite important matter - what about java 8 support?

@SergejIsbrecht
Copy link

Would like to see Java8 supported as well. As already discussed on twitter, it could probably be accomplished with https://blog.gradle.org/mrjars . I could have a look into it.

@yuriykulikov
Copy link

Hello,
is not a little bit too early for Java 8?
If Java 8 support is dropped, it will effectively mean that I will not be able to use Vavr in any of my projects (private and at work). Android is likely to be stuck on Java 8 for a long time.

@danieldietrich
Copy link
Contributor Author

I agree, supporting Java 8 should be the goal, especially because we do not use any Java 9 features besides module-info.java.

@SergejIsbrecht Java 8 is important for most Vavr users! Thx for volunteering, I'm already on it.

@talios @yuriykulikov@Opalo thx! ok, understood :) I will figure out how to support both Java 8 and native modules. But remember, the Vavr 1.0 API will break backward compatibility in several places.

However, when we support Java 8, the internal package will leak into the public API. We could move them to the parent package and make them package-private. But I don't know if we are able to hide them from the Java 9 module public API then. (Our modules are closed for reflection, but I haven't tested it, yet).

@emmanueltouzery

but it may become a hindrance if/when java gets pattern matching

Adding them later is possible without braking bw compat. I want to wait for the final patmat feature. Maybe we need to change the impls in order to get patmat working with Vavr.

@danieldietrich
Copy link
Contributor Author

I've thought about our options.

Building binaries for both Java 8 and Java 9

We don't use any other features from Java 9 than the module system. Our main-use case is not exposing the modules to the outside, this is a positive side-effect. The main use-case of adapting Jigsaw is to have a clean architectural design of our inter-package dependencies. More specifically, we benefit from Jigsaw during development time.

Because we 1) only use Java 8 API and 2) effectively only have an additional module-info.java of Java 9, we are able to produce Java 8 and Java 9 binaries from the same sources. Now, here we have two options:

  1. Create multiple versions of each jar, one for Java 8 and one for Java 9 (by using classifiers)
  2. Create multi-release jars, which contain Java 8 classes plus an additional module-info.class for Java 9+

The Gradle team looks critically at multi-release jars. This is because it is hard to reason about the behavior of an application when different versions (and different dependencies) are bundled at the same time. With multi-release jars the gap between source-code, modules and artifacts gets bigger.

However, we are in the comfortable situation that for both runtimes, Java 8 and Java 9, we use exactly the same source-code. Only the module-info.class is additionally used by the Java 9 runtime. Therefore I think a multi-release jar is (currently) the perfect solution for us. I don't see any drawbacks.

I will start to update the Gradle build accordingly.

Hiding implementation classes

Having an internal package in Java 9 that is not exported by the module solves the issue of hiding classes. However, putting the classes in a separate package (and making them therefore public) makes most sense when the classes are referenced from multiple packages.

In Java 8 such an internal package would leak the implementation classes to the outside, because they need to be public.

Therefore I think it is a better solution, to put make the implementation classes package-private and put them directly into the package of their interfaces/abstract classes. This will solve our original use-case of hiding the classes for both, Java 8 and Java 9.

Especially (I think, but have to re-evaluate it) these package-private classes are not discoverable via reflection in Java 9, if the module isn't declared as open.

@danieldietrich
Copy link
Contributor Author

@talios
Copy link

talios commented Mar 20, 2018 via email

@SergejIsbrecht
Copy link

@danieldietrich, will have a look at it after work today

@Opalo
Copy link
Contributor

Opalo commented Mar 20, 2018

In Java 8 such an internal package would leak the implementation classes to the outside, because they need to be public.

With internal package you can always warn the end users that this package's content may change at any time and you're not responsible for these changes 😉 Of course it doesn't make much sense and even though such packaging (I mean internal) is quite common it's a leaky abstraction and don't like it much.

Therefore I think it is a better solution, to put make the implementation classes package-private and put them directly into the package of their interfaces/abstract classes. This will solve our original use-case of hiding the classes for both, Java 8 and Java 9.

This (^) sounds really good. I'm wondering only if this is possible for all classes - no implementation will leak.

Also, I know that Java 8 and 9 builds have exactly the same source right now hence mrjar can be used. Will it change in foreseeable future? If so maybe it's better idea to prepare separate builds now and save some time in the future. No idea if this is a good suggestion, rather asking 😉

Let me know if you need any help with gradle.

@danieldietrich
Copy link
Contributor Author

@SergejIsbrecht great!

@Opalo

I'm wondering only if this is possible for all classes

We already do this for internal classes in Vavr 0.9. However, in Java 8 they are still discoverable using reflection. Namely changing internal classes might break backward compatibility. Java 9 modules to the rescue!

If so maybe it's better idea to prepare separate builds now and save some time in the future

In my experience this would be too early optimization. We currently do not need multiple versions of classes. This would add unnecessary complexity. We will add them if they are really needed.

Thx for all your thoughts and the feedback!

@danieldietrich
Copy link
Contributor Author

@SergejIsbrecht for now I fell back to a Java 8 only build. Please do not spend too much time on the Java8/Java9 multi-release jar. I think it might not work well for us or the build gets too complicated. For now a good old Java 8 build should be sufficient.

@SergejIsbrecht
Copy link

Ok, I will look into the suggestion of melix to generate binaries for each java version: java8 / java9 / java10. Probably use target Java10, because Java9 is not supported anymore. It would be great if we could hide internal implementation for >= Java9 and use internal package for Java8. I will play around a little bit and will give you feedback on this issue.

@danieldietrich danieldietrich changed the title VΛVΓ 1 (logbook / work in progress) Spike: modularization using Gradle Jul 31, 2018
@danieldietrich danieldietrich changed the title Spike: modularization using Gradle Spike: modularization Jul 31, 2018
@danieldietrich
Copy link
Contributor Author

Vavr 1.0 runs with Java 11+. I switched the master branch, it now contains the first module: vavr-control v1.0.0-alpha-1. More will follow soon (e.g. vavr-collection).

Java 8 will reach its end of life soon. Legacy applications are still able to use Vavr 0.9.x.

The original purpose of this issue (creating a spike) is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants