#1480 Use conditional packages for v7 release

thompson-tomo commented 8 months ago

Proposed Changes

With this change the polyfill package references have a condition placed on them so that they are only added for the net standard 2.0 TFM and not net 6.0.

Types of Changes

What types of changes does your code introduce to this project? Put an x in the boxes that apply

[x] New feature (non-breaking change which adds functionality) #1480

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask on the mailing list. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[x] I have read the CONTRIBUTING.md document
[x] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
[ ] All tests pass locally with my changes
[ ] I have added tests that prove my fix is effective or that my feature works
[x] Any dependent changes have been merged and published in related repositories

Further Comments

Closes: #1480

danielmarbach commented 8 months ago

Generally I would suggest grouping the private dependencies separately

  <ItemGroup Label="Private dependencies">
   ...
  </ItemGroup>

when it comes to having a TFM specific group I'd agree it makes sense under specific circumstances. The problem is though once you start moving things into TFM specific groups you can only use the API that is exposed for that version for that TFM. So for System.Threading.Channel you'd be seeing the API surface for the NET6 version and for Netstandard the API surface for the 8.0 version. That can lead to suttle differences that you might end up having to conditionally compile. For example System.Text.Json introduces default properties in the newer versions while the previous once don't have it. If you'd want to use such an API you have different API surface between different TFMs.

Maybe the important question is whether we want to benefit for those reference libraries from potential API surface improvements that we can leverage within the client that would otherwise not be available.

Given the age of NET6 I'd also argue that probably we'd want to bump to NET8 since this is for the main branch which isn't yet released.

thompson-tomo commented 8 months ago

The comment from @danielmarbach ties into the review thread I closed. I would propose a discussion topic is started to discuss when to up dependencies as currently a number of people have differing opinions.

In relation to the points raised I totally agree that we should try to avoid framework specific code where ever possible. After thinking about this a little bit more, perhaps we should tie dependencies to the Min version which can be provided by the framework ie 6.x and if functionality in a newer version is needed we add an additional TFM and make nuget fetch the library on older TFM.

But this PR focuses on what was mentioned in the issue which is to only add packages if not provided by the framework.

lukebakken commented 7 months ago

@thompson-tomo please see the following - https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/1482#pullrequestreview-1879040702

It appears that the concern @danielmarbach expressed has been uncovered in that PR.

thompson-tomo commented 6 months ago

Tests are now working, so have rebased

lukebakken commented 6 months ago

Thank you @thompson-tomo!

rabbitmq / rabbitmq-dotnet-client