eclipse / microprofile-reactive-streams-operators

Microprofile project
Apache License 2.0
80 stars 33 forks source link

Remove operators from PublisherBuilder #35

Closed jroper closed 5 years ago

jroper commented 6 years ago

From @venkats:

I feel that the methods in PublisherBuilder, like map and filter, are redundant since they're also in ProcessorBuilder. It may be better to make the PublisherBuilder a lot smaller and reuse the ProcessorBuilder more to build the pipeline?

jroper commented 6 years ago

It's true that they are redundant, the reason they are there currently is that it leads to a more fluent API, eg, this:

Publisher.of(1, 2, 3)
  .map(Object::toString)

vs this:

Publisher.of(1, 2, 3)
  .via(ReactiveStreams.<Integer>builder().map(Object::toString))

Nevertheless, conceptually it's a lot cleaner to always consider map a processor operation, and perhaps it's a good idea to view streams in terms of operations that get done in the middle, and then plumb them to inputs and outputs at the end. I think understanding PublisherBuilder and ProcessorBuilder both have map, but SubscriberBuilder doesn't isn't immediately obvious. If only ProcessorBuilder had map, it would be much clearer. The question is, can we come up with a fluent API that expresses this nicely?

hutchig commented 6 years ago

I think we need to tilt towards the expressive power of the API in use by apps doing functional composition over and above making it a 'better design' with less redundancy in the classes. -1 on this issue.

cescoffier commented 6 years ago

I agree, it leads to duplication but having a common API increase the usability.

OndroMih commented 6 years ago

To be honest, I agree with Venkat's comment and I don't really like the current solution, where ProcessorBuilder and PublisherBuilder share 90% of their methods, which differ only in the return type. And why is that? I think the only reason is that the messaging spec expects methods to return either ProcessorBuilder or PublisherBuilder. Why do we need that?

OndroMih commented 6 years ago

I think if we abstract the operators from the subscriber/processor/publisher concept, we can create a single interface with operators and reuse it, keeping it still fluent and simple:

ReactiveStreamBuilder<Processor,Object,Object> streamBuilder = ReactiveStreams.builder();

Processor<Object,String> processor = 
streamBuilder
   .map(Object::toString)
   .buildRs();

I know this won't compile but I don't have time to offer a final solution, I only wanted to demonstrate the idea: the builder method creates a reusable ReactiveStreamBuilder which has information about which type of object to create with the buildRs method.

Another approach would be the way of CompletionStage and CompletableFuture, where the latter implements all the methods of the CompletionStage interface, just overrides the return type to CompletableFuture. This would look like it is now, just with the common methods extracted to a superinterface, but still overridden in specific builders. I don't like it much, but still I like it much more than the current solution with redundant methods and no common interface.

Emily-Jiang commented 5 years ago

I like this proposal. I think the redundant methods should be removed.

cescoffier commented 5 years ago

Has been covered during the last refactoring #99

hutchig commented 5 years ago

Agree - happy for this to be closed.