Skip to content

Rethinking modularization of Vavr #2316

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

Closed
danieldietrich opened this issue Oct 16, 2018 · 19 comments
Closed

Rethinking modularization of Vavr #2316

danieldietrich opened this issue Oct 16, 2018 · 19 comments
Labels

Comments

@danieldietrich
Copy link
Contributor

danieldietrich commented Oct 16, 2018

This is the first question I ask on my own here (I think).

(Also see how Scala does modularization: here and here).

Log

For the last weeks I started to rewrite Vavr. The new code-base is influenced by actual versions of Scala and Java, and also by things I've learned from the past.

For example common interfaces at the root of our type hierarchy will disappear, like Lambda, Tuple and Value. The main reason is, that some methods, like Value.flatMap and Value.filter, could not be defined in a generic manner. Also some Value methods that make only sense for wrapper types (like Option et al) leaked into the collections API. A common 'placeholder' interface 'Lambda' for functional interfaces did not work out well on the type level. And Tuple encouraged the use of instanceof and type casts, which isn't the way we should code in a first place.

However, beside these relatively small changes, I started to modularize Vavr. The first module was vavr-control. But soon, the community began to question the modularization efforts, which seemed to serve the maintainers only. Beside other things, controls were missing 'rich' Iterators and methods like sequence() disappeared.

Meanwhile, I thought about removing Future but also started a poll on Twitter:

poll

It reminded me of the fact that it is okay that a major release introduces backward incompatibilities but it should not completely drop central functionality.

Question (@nfekete, @emmanueltouzery)

Therefore here is my question: How would you slice Vavr into modules?

Let's say, if the main Vavr lib still consists of the packages

  • io.vavr
  • io.vavr.concurrent
  • io.vavr.collection
  • io.vavr.control

The JPMS module name would be io.vavr. But that would prevent us from releasing other modules that share this package name, like the currently existing

  • io.vavr.gwt
  • io.vavr.kotlin
  • io.vavr.test

I think the simplest solution is to change the package of the main library:

  • io.vavr.core
  • io.vavr.core.concurrent
  • io.vavr.core.collection
  • io.vavr.core.control

What are your thoughts? How would it affect your existing code bases. What are your preferred migration paths?

There is also the option to stay on the classpath. But I think it is important to be at least 'JPMS-ready'.

Thx in advance!

@nfekete
Copy link
Member

nfekete commented Oct 17, 2018

Hey Daniel!

I'm not very familiar with JPMS. Anything that I say might not make sense at all.
It would be nice if vavr would be usable from both the classpath and the module path. Keeping Java 8 compatibility for the foreseeable future would be very important, almost 80% of devs are on Java 8. See the latest survey about the JVM ecosystem here: https://snyk.io/blog/jvm-ecosystem-report-2018
If JPMS module name collision requires us to use io.vavr.core for the main package, I think that's perfectly fine. It's just a name after all. The important thing is that it represents what it provides and that seems to be met.

As for what the individual modules should contain, where the module boundaries should be drawn, I think we should select a few fundamental things that should go into the core module, the non-fundamental things can go into their own modules. The fundamental things from my point of view are:

  • vavr (rich) Iterable(*) + Iterator
  • control structures: Option, Either, Try
  • function types
  • tuples
  • contract interfaces? (**)

*) We don't presently have this class, and I think, where it makes sense, we should make vavr classes implement a vavr Iterable. That is, a type that provides a "better" Iterator, a vavr Iterator. Value was problematic because it was in a way a kitchen sink that contained many functions that didn't really made sense for all subtypes. vavr Iterable as a supertype would be different in the sense that whenever you have a container that wraps 0 or more values, you could view it as something that you can iterate over. And io.vavr.Iterable<T> would contain only the io.vavr.core.Iterator<T> iterator() method, so that wouldn't be a kitchen sink as Value was.
**) By contract interface I mean interfaces that define a contract in a similar way as java.util.stream.Collector defines a contract to collect values. I'm not sure we would need such interfaces, but if we did, it would probably make sense to go into the core module.

@marknorkin
Copy link

Hello, @danieldietrich
I would like to give another point of view which is based on the poll provided by @nfekete.

I agree that most of developers currently use Java 8 and if vavr would require Java 9 this would also decrease the amount of potential users.
I would propose to take a look into Spring Framework JIRA ticket on automatic module names and approach they decided to go with.

If the vavr codebase stays on Java 8, this could be the option to be available for Java 9+ users on module path and in future gradually move vavr to Java 9 when most of the projects will migrate on it.

@emmanueltouzery
Copy link
Contributor

emmanueltouzery commented Oct 18, 2018

For me, I 100% support the removal of Value, for the reasons you and others listed. Another thing which I would change if API compatibility with vavr 0.9 is broken would be more type-safety. For instance sum() on collections which throws if the collection type is not a Number, or reduce which throws if the collection is empty.

The modularization... I didn't really research the topic, but I view java9 modules as something which is useful to modularize big systems.. like the JDK (which was done). Or possibly very large enterprise software (like the way some systems today use OSGI). Intuitively I didn't think about splitting vavr itself; vavr feels "small", especially since it doesn't pull any dependency itself.
Of the dependencies in our applications, vavr is among the smallest and definitely the highest value-for-size, so I didn't feel a need for modularization.

Actually to me the great added value of vavr and of FP libraries/preludes is the symmetrical/uniform API accross these concerns (Option, Either, collections, Future...). And I'm afraid that splitting them, this can be lost or hampered.

Certainly to me the loss of sequence would be a disaster. The loss of Future would be a real disappointment, although I understand that it's caused by implementation complexity, not modularity. I think ideally a thin wrapper around java's CompletableFuture with an API similar to scala's Future and other vavr classes would go a long way.

To summarize I think vavr 0.9 really hit a sweet spot for many people, maybe more than you may have realized as the author. I agree with some concerns, that it would be a shame to loose some of this to gain benefits for a very small minority of users which would actually potentially take advantage of the modularity (and the maintainers, although I of course sympathize with the load of work that is yours as the vavr maintainer).

@emmanueltouzery
Copy link
Contributor

PS: regarding the modules, honestly we use it all, except for kotlin, gwt, some collections like Trees or OrderedMaps. I mean honestly dependency size is not a concern, we would just pull it all.

PPS: another thing.. I see you'd drop Tuple and have some Entry class for map entries.. I just hope that Entry would also have map, map1, map2, transform like Tuple2 has.. Well as you can see I'm quite adverse to change in vavr... I really think it hit a nice sweet spot as it is...

@danieldietrich
Copy link
Contributor Author

Thank you all for your input, it is very valuable to me. I distilled it:

  • We will preserve backw.compat to Java 8
  • We will change the package name to io.vavr.core in order to have a unique module name but we will also stay on the classpath
  • We will remove Value and Tuple and keep Tuple1..N. Especially we will not introduce Map.Entry (/cc @emmanueltouzery )
  • The core will contain an uncluttered vavr (tuples, functions, controls, collections - and maybe concurrent.Future. In Scala concurrent will be an own module)
  • I will increase safety (e.g. reduce will not throw anymore, numeric operations might be removed or kept safe)
  • All types will be at least java.lang.Iterable (collections will be io.vavr.core.collection.Iterable) but all iterator() methods will return a 'rich' Vavr Iterator

I also thought about wrapping CompletableFuture in a Scala-like Future but I'm not sure, yet. I think our current approach might be fine for now.

@emmanueltouzery
Copy link
Contributor

Sounds great! Regarding tuples, Tuple2 might be enough. Tuple2 has a special role with Map in particular.

@danieldietrich
Copy link
Contributor Author

danieldietrich commented Oct 19, 2018

@emmanueltouzery / @nfekete / ...

Do you use Function1..N and/or Tuple1..N?

@nfekete
Copy link
Member

nfekete commented Oct 19, 2018

Tuple[2..3], Function[0..2] mostly. But I guess if we have Tuple[1..3] and Function[0..2]we should probably have them up to some reasonable number.

Regarding types being iterable, I think it's important to have io.vavr.core(collection.)?.Iterable at the top of the hierarchy, so that we can get a rich vavr Iterator out of these types without casting. For me, most naturally it would belong to the io.vavr.core package, it's not necessarily related to collections, collections just happen to all be iterables. Also, java Iterable is in the java.lang package.

@danieldietrich
Copy link
Contributor Author

@nfekete scala.collection.Iterable is different, it is at the top of the collection hierarchy. With Scala 2.13, Traversable will be deprecated and finally removed in Scala 2.14.

io.vavr.core.collection.Iterable currently is very much like Scala's scala.collection.Iterable. It has too many methods for Option, Try and Either. But I see the need for an interface at the top of the Vavr hierarchy, that has a rich iterator().

Instead of re-introducing the Value interface, we could in fact move a simplified version of Iterable (and the rich version of Iterator) to io.vavr.core. Additionally we would add a Java-like Collection interface to the top of the io.vavr.core.collection hierarchy. It would be a replacement for Vavr's Traversable resp. rich Iterable.

Then Vavr would look more like Java. 😅

@nfekete
Copy link
Member

nfekete commented Oct 19, 2018

I was mainly thinking about an iterable declared in a similar way:

package io.vavr.core;
public interface Iterable<T> extends java.lang.Iterable<T> {
    @Override
    io.vavr.core.Iterator<T> iterator();
}

I'm wondering about the usefulness of the map/flatMap methods. Do you have a use-case?
I think they would just cause problems further down in the type hierarchy.

@danieldietrich
Copy link
Contributor Author

danieldietrich commented Oct 19, 2018

@nfekete

I'm wondering about the usefulness of the map/flatMap methods. Do you have a use-case?

The current code base just contains example code. Iterable will soon look more like this, having 20-50 methods (not only map & flatMap).

Please don't confuse Vavr's current Iterable with Java's. It is a collection interface (in Scala and currently in Vavr). The collections in Scala always flatMap to Iterable. In fact this relaxes the Monad laws a bit but it is still okay. It also works well in Java/Vavr.

You have in mind that Iterable will be used not only by collections. This is what I meant with

(...) a simplified version of Iterable (...)

in the comment above. If we move Iterable to io.vavr.core, it will only have the iterator() method. map() and flatMap() will disappear. io.vavr.core.collection.Iterable will be renamed to io.vavr.core.collection.Collection. That one will have map() and flatMap().

Note: Our Traversable currently has flatMap(Function<T, Iterable<U>), which works well for collections.

Note 2: Iterable is the new Traversable in Scala 2.13.

@asarkar
Copy link

asarkar commented Nov 2, 2018

@danieldietrich I stumbled across this thread from gradle/gradle#5120, and after browsing 41cd78ff, have a couple of comments:

  1. It seems like you're structuring the modules following the Gradle official documentation, which for some reason, uses a very convoluted project to demonstrate a complex idea, thus ending in a mess. For one thing, and I'm speaking from experience, having a build.gradle in every module creates code duplication, and makes it very difficult to see the big picture at once. A single build file is much easier to maintain.
  2. The official recommendation is to name the module root directories the same as the modules, but you're not doing that. If my assumption is right that you followed the Gradle docs, then I see why not - they don't either. Conventions exist for a reason; why not follow them?
  3. I gather from the discussion here that vavr is not going to be JPMS-compatible overnight, but you can start by adding an Automatic-Module-Name attribute the MANIFEST.MF. That's the approach JUnit5 has taken, and I assume a lot of other libraries would too; it takes almost no effort on the maintainers' part.

I've created a sample project for my own learning that implements suggestion 1 and 2 from above; you're welcome to check it out.

@danieldietrich
Copy link
Contributor Author

Hi @asarkar,

thanks you for your feedback - here are my thoughts.

but you can start by adding an Automatic-Module-Name attribute the MANIFEST.MF

Thanks for your suggestion! We already do that in both, the current v0.9.2 and the upcoming v1.0.0.
I've experimented much with JPMS (in Maven and Gradle).

The official recommendation is to name the module root directories the same as the modules, but you're not doing that.

You read different things. I also stumbled across these docs. It is just a convention that has no effect regarding the builds. Some do it, some not. It does not matter.

For one thing, and I'm speaking from experience, having a build.gradle in every module creates code duplication, and makes it very difficult to see the big picture at once.

That's right. I started with one Gradle file. But Gradle's current ext.moduleName workaround for the Automatic-Module-Name approach requires each submodule to have a separate build.gradle.


Nevertheless, the link to the JPMS I gave you (in the Gradle issue on Github) is an old commit. The current version on the v1.0.0 branch does not have build.gralde files anymore in the submodules: https://github.com/vavr-io/vavr

Thx!

@asarkar
Copy link

asarkar commented Nov 2, 2018

It is just a convention that has no effect regarding the builds. Some do it, some not. It does not matter.

Those that do it includes the JDK, so AFAIC, the rest of the lot doesn't matter; YMMV. The funny thing about conventions is as soon as you create one, some people feel compelled to violate them for no apparent reason :)

Gradle's current ext.moduleName workaround for the Automatic-Module-Name approach requires each submodule to have a separate build.gradle

I suppose that's because your Gradle module names are not the same as the JPMS module names, hence you need a property. If those were the same, you could've just used the project name as the module name.

Thanks for maintaining the Vavr library, BTW, I use it extensively.

@danieldietrich
Copy link
Contributor Author

@asarkar

I suppose that's because your Gradle module names are not the same as the JPMS module names, hence you need a property.

I see! That's nice, thank you for the hint!

@jbduncan
Copy link

jbduncan commented Nov 24, 2018

Hi @danieldietrich - I realise that this goes against @emmanueltouzery's earlier opinion, but I'd find at least some modularisation to be a useful incentive for adopting Vavr; specifically, making at least the core and control packages their own module/jar and collections and concurrent a module/jar on top of that.

From my perspective, functional persistent collections are intriguing but I've yet to become comfortable with the idea of using them in any of my personal projects or introducing them to my colleagues, whereas I'm convinced of the value of Try, and I could possibly also be convinced of the value of Either and Option once I get to grips with them.

Do you have any thoughts regarding this? :)

@danieldietrich
Copy link
Contributor Author

@jbduncan Vavr is relatively small. All the past discussion showed that people are more willing to include just one lib (and maybe do not use parts of it) instead of having the overhead of including several libs. It complicates things.

Also, we need to throw away existing features (like Try.sequence) when shipping control as separate module.

Therefore my current strategy is to keep things backward compatible if possible (like package structure, naming etc) and just fix wrong design decisions in order to get Vavr right (like type hierarchy etc).

@jbduncan
Copy link

Okay, those are totally fair reasons to avoid splitting up Vavr! Thank you very much for responding to my comment. :)

@danieldietrich
Copy link
Contributor Author

Thanks

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

No branches or pull requests

6 participants