was Uni.replaceWith(Supplier) a mistake

[gavinking]

I hate opening this issue, since it's something I had already considered from one point of view when I originally proposed adding the replace() method, and decided was OK, but that I'm now thinking might have been a mistake.

So originally I thought of it this way. When I looked at replaceWith(Supplier) I immediately thought that:

1. it looked more like an overload of map() than an overload of replaceWith(), and
2. it was a bit weird having replaceWith(Supplier<T>) and replaceWith(Uni<T>) but no replaceWith(Supplier<Uni<T>>).

But:

1. by its very name, map() should be mapping something to something else, not supplying something, and
2. the "replaceWith(Supplier<Uni<T>>)" method actually already exists and is called chain().

And, it seemed clear that replaceWith(Supplier<Uni<T>>) was useful, so I included it in the proposal, and nobody complained.

However, after having now actually used this stuff in anger, and having tripped up on thinking of replaceWIth() as a sort of chain(), which it's definitely not, I'm wondering if we need to make it more clear that replaceWIth() is an operation that can't depend on side effects of previous stages.

Having replaceWith(Supplier<T>) there makes it a lot less clear that the other two overloads should never be used when their argument depends on the completion of a previous stage, and I feel like users are sure to trip up on that.

I'm not saying we should necessarily fix this, but I wanted to draw your attention to my mistake, so we can at least discuss it.

Sorry about this.

[gavinking]

By the way, after considering this, together with #318, I think that probably the cleanest API would have been for Uni and UniOnItem to both offer all the following operations:

• replaceWith(Y) and replaceWith(Uni<Y>)
• transform(Function<X,Y>) and transform(Supplier<Y>)
• chain(Function<X,Uni<Y>>) and chain(Supplier<Uni<Y>>)
• invoke(Consumer<X>) and invoke(Runnable)
• call(Function<X,Uni<?>>) and call(Supplier<Uni<?>>)

Where:

• the existing map() and flatMap() on Uni would be aliases for transform() and chain() respectively, and
• one or perhaps two of the overloads of UniOnItem.transformToUni() would be deprecated in favor of chain(), since they are the exact same thing and it's confusing that they have different names on the two interfaces.

Again, I'm not saying we should do this, I'm just saying I now realize it would be the cleanest and most consistent organization of these methods, and it would fix both #319 and #318.

[gavinking]

In my defense, the UniOnFailure.recoverWithXxxx() methods suffer from the same "mistake", which I was also aware of when I was writing the proposal.

[jponge]

Thanks @gavinking, this is a good set of thoughts. I see where you are going, what you want is a clean separation between:

1. value-transforming operations, possibly accepting a Supplier<T> when you want to discard the item, and
2. operation-chaining, possibly accepting a Supplier<Uni<?>> when you want to discard the item.

I have tried to summarize the proposed changes as follows, please correct me if I'm wrong. Comments in nested bullets.

• Discard Uni.replaceWith(Supplier<T>) in Uni
• Add transform to Uni with Function and Supplier overloads
• Keep chain, call and invoke as-is in Uni and UniOnEvent
• Add transform, transformToUni and transformToMulti overloads in UniOnEvent with Suppliers of values (not Supplier<Uni<?>>)
  • Since these variants would drop the item I'd suggest replaceWith, replaceToMulti and replaceToUni
  • Alternatively: replaceWith, discardToMulti and discardToUni
• Replicate chain of Uni in UniOnEvent
  • Aren't we creating a feature envy here?
  • Instead of uni.onItem().chain(...), don't we simply want uni.chain(...) here?
• Replicate replaceWith(T and Uni) of Uni in UniOnEvent

[jponge]

BTW @gavinking could you please point me to some snippets where these API changes would help?

I'm not necessarily interested in all, just 1-2 relevant ones. Thanks!

[gavinking]

what you want is a clean separation between:

what you want is a clean separation between:

1. value-transforming operations, possibly accepting a Supplier<T> when you want to discard the item, and
2. operation-chaining, possibly accepting a Supplier<Uni<?>> when you want to discard the item.

Well, it's more of a question: do we need such a clean separation? The issue is that I think I've seen that it's potentially dangerous to have the replacing value be evaluated at stream-construction time. I mean, it's not conceptually wrong, it's just that if that semantic isn't completely clear to the user, then they could be in for nasty surprises. Now, whether the naming really helps make that clear (or more clear than it already is from the signature) I'm not sure.

I'm not sure that your summary exactly captures what I'm saying (which I hesitate to call a proposal, it's more of a straw man, and I'm not actually advocating that we do it) so let me clarify a bit. I apologize in advance. This is going to be long-winded.

First of all, I want to say that I think I've finally nailed down precisely why I was uncomfortable with the older revisions of the Mutiny API: I was continuously forced to write onItem() when I just didn't have any item: I was often working with Uni<Void> to chain side-effecty computations, not to transform streams of values. From that point of view all the onItem() calls, apart from being very verbose, were also kinda misleading about what was going on. With the current revision of the API, that discomfort is almost completely gone: i.e. it was really the separation of Uni and UniOnItem that I had a problem with, not any of the other UniXxxxx interfaces, which quite nicely model "special" states of the stream.

Anyway, that's a long way of saying I've grown comfortable with UniOnFailure and friends but I still have trouble convincing myself about what essential role UniOnItem fills. I mean, I'm very OK with it having more specialized methods related to processing the item that don't belong on the main Uni and that's fine. But then I'm finding it sorta difficult to mentally model the real distinction between these types, given the restriction that I don't want to force people to call onItem() all the time for basic things.

So in response to your comment about "feature envy" @jponge, well, I guess my response is that they should share a basic set of operations because AFAICT they're not very different in terms of what they model.

Now, sure, if you wanted to be completely disciplined about enforcing the paradigm of having a type to represent each distinct "stream state", then I guess you could say that we should eliminate versions of chain() and call() and invoke() which accept an item, leaving only those which ignore the item, and make you call onItem() if you want access to it. That would leave you with a very clean separation between these types. But the cost of that is a lot of verbosity and people winding up finding map() and flatMap() just too tempting, and .... well, we've been through all this already and I don't think we should revisit that. I think the current API is much more enjoyable to use than what was there earlier and I wouldn't want to go back.

So my suggestion is that we should consider regularizing the operations shared between Uni and UniOnItem, instead of having to remember that Uni.map() is the same thing as UniOnItem.transform() and Uni.chain() is UniOnItem.transformToUni() etc. (Again, I'm not strongly advocating this, I'm just putting it out there.)

To my list:

• replaceWith(Y) and replaceWith(Uni<Y>)

Here I'm removing (actually renaming) one overload of replaceWith() on Uni.

Also I'm copying the operation to UniOnItem. But perhaps that's a bad idea. Why would you be calling onItem() just to throw away the item?

• transform(Function<X,Y>) and transform(Supplier<Y>)

This is the same operation as Uni.map() and UniOnItem.transform(), plus the overload of replaceWith() that I've removed above.

• chain(Function<X,Uni<Y>>) and chain(Supplier<Uni<Y>>)

These are just the existing operations on Uni, which I think are great, but I'm suggesting replicating them to UniOnItem.

• invoke(Consumer<X>) and invoke(Runnable)
• call(Function<X,Uni<?>>) and call(Supplier<Uni<?>>)

These we already have everywhere.

So what this naming largely achieves is separation of:

1. operations which return Unis from operations which return values or void (this is needed b/c of Java overload resolution rules), and
2. operations with arguments that might be evaluated at stream-construction-time.

It doesn't separate operations which process the item from the previous stage from operations which ignore it, and I think that's completely OK because it's just so obvious from the signature. @jponge, I think that was something you wanted to do, but I'm not convinced it's necessary or helpful.

Anyway, these are just thoughts.

A further thought that only just occurred to me. Again, I which I had thought of this sooner is: perhaps none of the operations which ignore the item belong on UniWithItem! Why would you call onItem() to throw it away?