search

smallrye / smallrye-reactive-streams-operators

Implementation of the MicroProfile Reactive Streams Operators specification
https://www.smallrye.io/smallrye-reactive-streams-operators/
Apache License 2.0
20 stars 9 forks source link

Uni type feedback #87

Closed emmanuelbernard closed 5 years ago

emmanuelbernard commented 5 years ago

Emmanuel is a noob in reactive things. Feel free to ignore the proposals that are not possible for fundamental reasons.

Types

I hate packages with the name api.

UniSubscriber and UniSubscription do not receive nor return Uni in their signature. Can we therefore have generic Subscriber / Subscription? We can have a prefix to differentiate them from others like Rx, I'm arguying for a unified Uni and Multi Subscription types. Are they used by normal people? If no, maybe put them in a sub package.

Disposable does not seem to be used from a user PoV (from the tests I am seeing), do we really need it? In the main package?

Methods

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/UniEmitter.java#L28 Is it common enough to call success() to warrant an extra method exposed?

I personally find asFoo() more natural to read than toFoo(). I don't know if there is a strong reason to use to.

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L271 I'd rename CompletableFuture<T> subscribeToCompletionStage(); to CompletableFuture<T> subscribeAndConvertToCompletableFuture(); While toFoo() might be readable (though I find asFoo more readable), subscribeTo changes the meaning.

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L163 I'm not super clear what it defers to from the javdoc, nor when I would want to use it.

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L350 I don't understand what that one does.

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L377 When do you plan to use this one? Having to pass a function instance seems annoying at first value compared to a static Converter.class of sort.

Wild proposals

Note that all this design does not requires several implementations, DefaultUni can implement all these types returned by subscribe(), from() etc and this is returned.

TODO (Emmanuel):

  1. Look at and discuss to.
  2. Consider doing the method aggregation pattern for operators like or().else(), retry, on().error().return() etc
  3. Also offer a generic operator extension model based on the uni from(Converter) proposal above (Bean Validator pattern).

Details

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/UniEmitter.java#L6 => s/single signals/single signal/

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L37 => s/being subscribed to the specified value if/being subscribed to with the specified value if/

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L84 => s/If it's not the case the subscriber's callbacks/If it's not the case, the subscriber's callbacks/

Implementation detail

It's useful to have UniFailed UniOf etc, vs a generic Uni impl and passing the behavior as lambdas in the constructor?

emmanuelbernard commented 5 years ago

@cescoffier @FroMage here are my first batch of feedback. I don't know how readable my wild proposals are, so feel free to ask questions.

cescoffier commented 5 years ago

I hate packages with the name api.

That's temporary. Actually, I will extract this to a new repo / project.

UniSubscriber and UniSubscription do not receive nor return Uni in their signature. Can we therefore have generic Subscriber / Subscription? We can have a prefix to differentiate them from others like Rx, I'm arguying for a unified Uni and Multi Subscription types. Are they used by normal people? If no, maybe put them in a sub package.

MultiSubscription should be a Reactive Streams Subscription. Also, MultiSubscriber should be a ReactiveStreams Subscriber, so no new class required for these. Here we need a specific Subscription because the request method (that matter for multi-item streams), is not used. That said, I agree that these classes are rather internal / advanced users and most people won't use these classes directly. So I'm in favor in moving them (we can even remove UniSubscription)

Disposable does not seem to be used from a user PoV (from the tests I am seeing), do we really need it? In the main package?

Definitely in implementation details. Should not be there.

Methods https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/UniEmitter.java#L28 Is it common enough to call success() to warrant an extra method exposed?

I was wondering about this one actually. And I'm still puzzled. I believe that null should be an explicit decision and so this method (that pass null) should be dropped. However, I wanted to discuss it with @FroMage.

I personally find asFoo() more natural to read than toFoo(). I don't know if there is a strong reason to use to.

I've been using to to avoid confusion with the as is a Kotlin keyword(https://kotlinlang.org/docs/reference/typecasts.html#unsafe-cast-operator). But I don't have a strong opinion about it.

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L271 I'd rename CompletableFuture subscribeToCompletionStage(); to CompletableFuture subscribeAndConvertToCompletableFuture(); While toFoo() might be readable (though I find asFoo more readable), subscribeTo changes the meaning.

Agreed, it should be renamed.

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L163 I'm not super clear what it defers to from the javadoc, nor when I would want to use it.

Yes, I need to improve the Javadoc. It's actually quite useful, as it let you postponed the creation at subscription time. So you would create the resources only if required (and not at instantiation time).

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L350 I don't understand what that one does.

Again, the Javadoc is not great... Imagine 2 async actions running in parallel, you want to be notified when both completed but without requiring their results. That being said, this is close to the zip operator (but the results are not discarded). So maybe we can remove it.

https://github.com/smallrye/smallrye-reactive-streams-operators/blob/features/POC-Uni/implementation/src/main/java/io/smallrye/reactive/streams/api/Uni.java#L377 When do you plan to use this one? Having to pass a function instance seems annoying at first value compared to a static Converter.class of sort.

Agreed, we can remove it. It was before adding the adapters.

Wild proposals Replace fromPublisher(Publisher) and fromPublisher(PublisherBuilder) with:

either 1. Uni from(Class).foo(Foo) used like Uni Uni.from(PublisherConverter.class).publisher(publisher); where publisher(P...)is a specific method driven directly or indirectly by thePublisherConverter`. Examples of APIs using this pattern is https://github.com/beanvalidation/beanvalidation-api/blob/master/src/main/java/javax/validation/Validation.java#L63 https://github.com/beanvalidation/beanvalidation-api/blob/master/src/main/java/javax/validation/Validation.java#L152 https://github.com/beanvalidation/beanvalidation-api/blob/master/src/main/java/javax/validation/spi/ValidationProvider.java#L26 (I think it can be simplified for our use case though) or 2. Uni from().publisher(Publisher) Uni from().publisher(PublisherBuilder) where these methods are "hardcoded" in the return type of from() This makes for an easier usage for the known type to convert from. We can have a from(Class) (see option 1) for types "unknown" like Rx1, Rx2, Reactor etc or 3. (variant of 2.) fromPublisher().with(Publisher), fromPublisher().with(PublisherBuilder), fromConverter(Class) The idea is to reduce the methods displayed by the IDE and be extensible for each of these >alternatives Same for fromOptional

I would prefer 2. Right now, we don't have the Converter class, it's managed using: from(I instance) and this triggers a lookup for the right UniAdapter instance (SPI). For instance:

Mono<String> mono = ...
Uni<String> uni = Uni.from(mono);

It also has the opposite variant (using to so might we as):

Uni<String> uni = ...
Mono<String> mono = Uni.to(Mono.class)`

So basically it hides the Converter/Adapter class.

Replace failed(Throwable) and failed(Supplier) with failing().with(Throwable) and failing().with(Supplier) The idea is to reduce the methods displayed by the IDE

I need to try to be sure it has the "expected reduction of methods displayed in your IDEin the sense that this should be done for the complete API (failing,on,collect,from,block`...). It worths a try!

Replace subscribe(UniSubscriber) UniSubscription subscribe(Consumer, Consumer) and > CompletableFuture subscribeToCompletionStage() with subscribe().withSubscriber, subscribe().withCallbacks, subscribe().andReturnCompletableFuture() Replace block* with block().indefinitely(), block().withDuration(), block().andReturnOptional()

Note that all this design does not requires several implementations, DefaultUni can implement all >these types returned by subscribe(), from() etc and this is returned.

Definitely worths a try.

TODO (Emmanuel):

Look at and discuss to. Consider doing the method aggregation pattern for operators like or().else(), retry, >on().error().return() etc

Wasn't return a keyword ;-)

Also offer a generic operator extension model based on the uni from(Converter) proposal above >(Bean Validator pattern).

So, that was proposed as from and to using:

static <T, I> Uni<T> from(I instance);
<O> O to(Class<O> clazz);

Now the question is whether we want to make the converter/adapter explicit.

Implementation detail It's useful to have UniFailed UniOf etc, vs a generic Uni impl and passing the behavior as lambdas in > the constructor?

Why not.

FroMage commented 5 years ago

My feedback:

emmanuelbernard commented 5 years ago

I don’t think the as keyword from Kotlin will be a problem. It’s a keyword and therefore is not a (chainable) method

Ladicek commented 5 years ago

The idea is to reduce the methods displayed by the IDE

Quite often, especially in APIs like this (java.util.stream, RxJava, etc.) I let my IDE show me all possible methods and I look at their signatures (especially return types) to see if there's something that fits what I need to do. Please don't take that away.

I don’t think the as keyword from Kotlin will be a problem. It’s a keyword and therefore is not a (chainable) method

If there's a method called as on a Java class, you can't call it from Kotlin like obj.as(...), because as in Kotlin is a non-contextual keyword (https://kotlinlang.org/docs/reference/keyword-reference.html). You have to do obj.`as`(...).

cescoffier commented 5 years ago

Closing as Uni has been moved to its own repo.