dotnet / reactive

The Reactive Extensions for .NET
http://reactivex.io
MIT License
6.73k stars 751 forks source link

Add ToImmutable* extension methods to System.Linq.Async #2164

Open idg10 opened 3 months ago

idg10 commented 3 months ago

This adds extension methods for IAsyncEnumerable<T> that offer conversion to various immutable collection types such as ImmutableList and ImmutableHashSet.

This takes the work done by Tau Gärtli in https://github.com/dotnet/reactive/pull/1545 and does the necessary updates to enable it to be merged in with the current state of main.

We weren't able to bring that PR in directly because it was 3 years old, and a lot of other things had moved on around it.

Since the PR had been neglected due to this repo effectively finding itself without owners for a while. When endjin took over maintenance our priority was Rx, and since the .NET team sometimes suggests they might write their own replacement for System.Linq.Async, we have prioritised other work, so this went unheeded for about 3 years!

Reasonably enough, @bash doesn't really want to do any more work on this PR. Understandable, given how it has been neglected for years.

But we don't want the work to be wasted, so we would like to get this work merged. This means bringing it back into alignment with the changes that have been necessary to keep everything building on current SDK versions.

So what we did was merge the original PR into a new feature branch. We then merged main into that feature branch to get it back up to date, and fixed all the issues that arose as a result of attempting a merge 3 years after the code was written. I've also updated the tests we have that verify that we don't accidentally change the public API surface area—we need to do that any time we add new APIs. And I've added release notes.

Note: before we merge this, we'll need to go through @bartdesmet 's feedback on the original PR. I think where this ended up with namespaces and packaging is OK. (It's certainly fine for .NET 6+ because those all have the immutable collection types built in.) The one question mark is over how we feel about making System.Linq.Async force existing .NET FX projects or anything relying on .NET Standard 2.x to take a dependency on the immutable collections library.

idg10 commented 3 months ago

Just to bring out the System.Collections.Immutable dependency issue, here's the situation:

Given the wild west of modern dependency trees, this might not be too big a deal, but it's worth noting that today, the net48 and netstandard2.0 targets of this library have just 1 dependency, and the netstandard2.1 target has no dependencies. (The net6.0 one really should have none either, but due to an oversight, it depends on Microsoft.Bcl.AsyncInterfaces. That has been fixed, and whenever we next do a release of System.Linq.Async, the net6.0 target will also have zero dependencies.)

The Rx and Ix libraries have historically been low-dependency, so it's slightly sad to move away from that.

However, for modern runtimes (.NET 6.0, 8.0, and whatever comes next), this change will continue to enable zero dependencies. So this really only affects .NET Framework and anyone still depending on .NET Standard (e.g., UWP).

The question is: how much do we care about imposing a System.Collections.Immutable dependency on people using System.Linq.Async on those older runtimes?

Also, what is the alternative? There are two.

Option 1: don't offer these methods on older platforms

We could just have these new methods be compiled in conditionally so that they don't appear on platforms that don't have System.Collections.Immutable built in.

This would sort of be consistent with the overall goal that System.Linq.Async gives you the same extensions for IAsyncEnumerable<T> that are built in for IEnumerable<T>. The ToImmutableList extension for IEnumerable<T> is not actually built into .NET Framework. But it would also be kind of inconsistent: if you've added a reference to the System.Collections.Immutable package, the extension method for IEnumerable<T> will be available.

Option 2: put these extension methods in a separate package

I'm reluctant to do this because I think people probably won't discover them. And even if they do, they may be reluctant to add an extra dependency for a fairly marginal feature. This also penalizes people who are using .NET 6, 8, or later, for no real reason.

So I think the least bad option is probably to go with what's in this PR: we impose the System.Collections.Immutable dependency on people using old frameworks. This gives anyone targeting current .NET the best experience.

Romfos commented 3 months ago

Context for the second half of 2024:

in general this is a good discussion point:

my private opinion:

bash commented 2 months ago

Thank you for adopting my PR and pushing this forward 💛