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

Could we have Tuple Overload for Zip, CombineLatest, and any other combine type method? #1269

Closed
Thaina opened this issue Aug 20, 2020 · 8 comments
Milestone

Comments

@Thaina
Copy link

Thaina commented Aug 20, 2020

Feature request

It would be more comfortable to work with Zip and CombineLatest to let it generate Tuple by default when we don't supply result function

IObservable<L> lStream;
IObservable<R> rStream;
lStream.Zip(rStream).Do((pair) => Console.WriteLine(pair)); // pair is ValueTuple<L,R>

Is it possible?

@quinmars
Copy link
Contributor

Since there is a IEnumerable<> counterpart in dotnetcore for that, it is probably a good idea to add at least the Zip variant to keep symmetry with IEnumerable<>.

@bartdesmet
Copy link
Collaborator

Agreed. The symmetry is worth having and extending to CombineLatest and WithLatestFrom as well. We're happy to take a community contribution for this. It'd have to be complete though, i.e.:

  • For Zip, CombineLatest, and WithLatestFrom.
  • All overloads are provided.
    • E.g. Zip has 16. So tuple names would be First, Second, ..., Sixteenth.
    • There's also a Zip overload that has IObservable<> and IEnumerable<>.
  • Also added in Qbservable to keep parity with Observable.

Example:

public static IObservable<(TFirst First, TSecond Second)> Zip<TFirst, TSecond> (this IObservable<TFirst> first, IObservable<TSecond> second);

We have to match the casing (First rather than first) and use tuple syntax (rather than ValueTuple<...> types directly), so the component names flow.

Also, right now, these methods also use TSourceN where N varies from 1 to 16. Because names for tuple-based overloads leak into the tuple component names, we'll have to use the proper English ordinals, prefixed with T for the generic parameters.

One caveat I can see at this point is that we'll need some #if in the code because ValueTuple<> isn't supported in all platforms Rx is still targeting today. @clairernovotny, any thoughts on our target platforms strategy? I'm happy to add more feature flags in the .targets file (i.e. HAS_VALUE_TUPLE) but we may need to refine the platform matrix a little bit because there's e.g. .NET 4.6 but no .NET 4.7 (which has these types). For this feature to be enabled we'd need any of these:

  • .NET Framework 4.7 or above
  • .NET Standard 2.0 or above
  • .NET Core 1.0 or above
  • .NET 5 (or above)

Maybe, once .NET 5 ships, we can tighten the target platforms matrix a little bit? (E.g. I'd be happy to boost .NET Framework minimum requirements for Rx vNext to v4.7.2 where a lot of issues were addressed IIRC.)

@bartdesmet
Copy link
Collaborator

bartdesmet commented Sep 25, 2020

Preliminary work to make adding overloads easier is done in #1302.

@bartdesmet
Copy link
Collaborator

bartdesmet commented Sep 25, 2020

There's an unfortunate breaking change that needs to be pondered before we go ahead:

public static IObservable<IList<TSource>> Zip<TSource>(params IObservable<TSource>[] sources)

If an overload is added for tuples, such as:

public static IObservable<(TFirst First, TSecond Second)> Zip<TFirst, TSecond>(this IObservable<TFirst> first, IObservable<TSecond> second)

then code like this (where xs and ys have the same element type):

Observable.Zip(xs, ys)

encounters a change in return type (from IObservable<IList<T>> to IObservable<(T, T)>), thus breaking user code.

The same issue occurs for CombineLatest.

@clairernovotny
Copy link
Member

Now is a good time for a breaking change. I bumped the ver to 5.x in #1291 since I had to also rename DispatchObservable to CoreDispatchObservable (along with ScheduleOnDispatcher -> ScheduleOnCoreDispatcher and ObserveOnDispatcher -> ObserveOnCoreDispatcher) since .NET 5.0-windows supports both in the same project.

If we need to do more breaking changes, I think it's a good time to do it.

@bartdesmet
Copy link
Collaborator

We can't really take away the params-based overload; there is no good alternative for it. Anyone using this overload (that's been there since day 1) will have been writing Observable.Zip(xs, ys), for any number of sources, and always get an IObservable of lists. When adding these new variants, the old behavior would show up when specifying more than 16 sources (or whatever number of overloads we support), but not when specifying less than 16.

The only alternative I see right now is to put the new overloads in ObservableEx (or whatever name). They're still discoverable as extension methods. But Observable.Zip won't bind to the new overloads, this preserving compat.

Nothing is more frustrating than upgrading a dependency and seeing your code (possibly more than 8 years old in this case) break in a random place. It's twice as bad if there's no alternative to restore the old behavior. So let's proceed with caution and put them on a different type. Still extension methods, so most people won't notice. Only when calling them as static methods does the ObservableEx (or whatever) type show up.

@bartdesmet
Copy link
Collaborator

I'll prepare a PR later this weekend, time permitting.

@bartdesmet
Copy link
Collaborator

This is checked in now.

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

4 participants