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

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

idg10
Copy link
Collaborator

@idg10 idg10 commented Aug 28, 2024

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 #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
Copy link
Collaborator Author

idg10 commented Aug 28, 2024

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

  • for .NET 6.0 and later there's no issue because these types are built into the framework
  • .NET FX 4.8 does not have these, so this change means System.Linq.Async would now cause a dependency on System.Collections.Immutable
  • These types are not in .NET Standard either, so our netstandard2.0 and netstandard2.1 targets also would get an extra dependency on System.Collections.Immutable

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
Copy link

Romfos commented Aug 31, 2024

Context for the second half of 2024:

  • most of developers are using .NET 6/8 as target platform

in general this is a good discussion point:

  • What versions should be supported? Do we care about unsupported versions of frameworks?
  • What priorities for them? Do we really should focus on OLDER platform, to make optimal solution for this, or maybe better to make sense to be focused on more popular and modern platforms?

my private opinion:

  • Make sense to have .NET6/8/9 as primary target platforms and support more features on these platforms like nullable reference types, spans, code generators, modern c# feature like records, immutable types e.t.c (maybe not all of the are super relevant for this specific library, but this is general idea)
  • .NET Framework is still make sense to support, but as a second priority
  • In case of conflict (as we have it here) make sense to choose more optimal solution for modern .NET rather then old .NET Framework

@bash
Copy link
Contributor

bash commented Sep 3, 2024

Thank you for adopting my PR and pushing this forward 💛

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants