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

Remove polyfill packages #2057

Closed
thompson-tomo opened this issue Apr 12, 2024 · 7 comments
Closed

Remove polyfill packages #2057

thompson-tomo opened this issue Apr 12, 2024 · 7 comments

Comments

@thompson-tomo
Copy link

thompson-tomo commented Apr 12, 2024

Is your feature request related to a problem? Please describe.
I want to minimise dependencies in my project by utilising framework dependencies wherever possible

Describe the solution you'd like
In Polly.Extensions System.Diagnostics.DiagnosticSource can be provided via the framework ie net 6/8 as such conditions should be placed on it so that it is only included for the other frameworks. Other dependencies can be optimised.

Describe alternatives you've considered
Accept the additional dependency

Additional context
n/a

@martincostello
Copy link
Member

I'm lead to believe that removing dependencies is a breaking change (in fact, I hit this today in #2003 due to changes to Microsoft.Extensions.TimeProvider.Testing dropping seemingly redundant dependencies), so if that's the case this isn't something we'd do until there's a reason for a Polly v9 if we did.

@thompson-tomo
Copy link
Author

In the case here I am referring to polyfill package's ie ones which are natively provided by the framework without it needing to be explicitly added hence I don't see it as a breaking change.

@martincostello
Copy link
Member

martincostello commented Apr 12, 2024

Yes I understand that, and this comment is what lead me to my comment: dotnet/extensions#5058 (review)

And also the fact that it actually broke the build in #2056 when I updated to Microsoft.Extensions.TimeProvider.Testing 8.4.0 without me adding the reference.

@thompson-tomo
Copy link
Author

I am still talking about polyfill package's and as such if a client is using System.Diagnostic.DiagnosticSource in their application/library it will still build as the framework will provide the library and likely even provide a newer version.

@martintmk
Copy link
Contributor

The System.Diagnostics.DiagnosticSource is only included on netstandard2.0 net472 net462 and we cannot really use the one that is provided by .NET Framework. This is because we are using APIs that are only available on newer version of the library.

If I add System.Diagnostics.DiagnosticSource as per your suggestion:

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Options" />
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
    <Reference Include="System.Net.Http" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'" />
    <Reference Include="System.Diagnostics.DiagnosticSource" Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'" />
  </ItemGroup>

The Polly.Extensions cannot be built anymore:

image

@thompson-tomo
Copy link
Author

@martintmk let me submit a PR tomorrow my time showing the change as your sample code is not quite what I am intending.

@thompson-tomo
Copy link
Author

Issue has been addressed already as part of the 8.4.0 release which has occured, hence closing issue

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

3 participants