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

Avoid spurious references to Microsoft.Bcl.AsyncInterfaces #1719

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

WeihanLi
Copy link
Contributor

Update System.Linq.Async dependency condition for Microsoft.Bcl.AsyncInterfaces

@WeihanLi
Copy link
Contributor Author

Is there someone having a look at this wrong dependency condition 👀

@danielcweber
Copy link
Collaborator

@idg10 May I kindly ask to give this PR a quick and final review, it fixes a unnecessary dependency on Microsoft.Bcl.AsyncInterfaces on platforms where it's not needed due to a wrong condition on the dependency. Thanks a lot.

@danielcweber danielcweber requested a review from idg10 October 13, 2023 12:17
@idg10
Copy link
Collaborator

idg10 commented Oct 13, 2023

There doesn't seem to be a clear discussion of what problem this sets out to solve, so I'm going to attempt to reverse engineer the intention from the PR.

The existing code was supposed to embody this idea:

Modern targets have IAsyncEnumerable<T> and related interfaces built in, so they don't need a reference to Microsoft.Bcl.AsyncInterfaces.

Unfortunately, this was expressed by equating "modern targets" with ".NET Core 3.1 and .NET Standard 2.1". At the time that code was written, those were indeed the most modern targets.

However, those targets are now, respectively, "old, and out of support" and "still current, but of decreasing relevance because the 'one .NET' initiative means that .NET Standard 2.1 is no longer very interesting."

Because those definitions of "modern targets" were not updated in this particular file, the problem now is that actually modern targets (and in fact anything "modern" enough to be a currently supported version of .NET) get misidentified as "not modern" and so they get this spurious reference.

This PR resolves this by flipping the logic, so that it works this way:

Old targets don't have IAsyncEnumerable<T> and related interfaces built in, so they need a reference to Microsoft.Bcl.AsyncInterfaces.

The set of "old targets" here is small, and known, and it won't change. It's just: .NET Framework 4.8 and .NET Standard 2.0. Those are the only targets we'll ever support that will have this requirement, so the advantage of flipping the logic this way is that this logic will continue to be correct as future versions add newer targets. So whereas the existing code has demonstrably gone stale, with the effect that we get spurious references to Microsoft.Bcl.AsyncInterfaces in projects that don't need it, the change will not require any further updates as we add new targets in the future.

@idg10
Copy link
Collaborator

idg10 commented Oct 13, 2023

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@idg10 idg10 changed the title Update dependency condition Avoid spurious references to Microsoft.Bcl.AsyncInterfaces Oct 13, 2023
@idg10 idg10 merged commit 6499c8e into dotnet:main Oct 13, 2023
4 checks passed
@danielcweber
Copy link
Collaborator

Thanks a lot. There was a bit of discussion in #1941 which I then closed in favor of this earlier PR.

@WeihanLi WeihanLi deleted the patch-1 branch October 13, 2023 15:39
@masonwheeler
Copy link

Now that this is merged, can we expect an updated NuGet release anytime soon?

@wessleym
Copy link

wessleym commented Apr 5, 2024

@danielcweber, @idg10, and @WeihanLi, sorry to bother you, but I'm also interested in @masonwheeler's question. I would like to see a NuGet package release so I can avoid the Microsoft.Bcl.AsyncInterfaces dependency in my .NET 8 projects. Thank you.

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

Successfully merging this pull request may close these issues.

5 participants