-
Notifications
You must be signed in to change notification settings - Fork 595
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
#1480 Use conditional packages for v7 release #1481
#1480 Use conditional packages for v7 release #1481
Conversation
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. |
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. |
9fb5caa
to
7818434
Compare
7818434
to
e4a8f3a
Compare
@thompson-tomo please see the following - #1482 (review) It appears that the concern @danielmarbach expressed has been uncovered in that PR. |
e4a8f3a
to
ee9b9a4
Compare
ee9b9a4
to
1983490
Compare
Tests are now working, so have rebased |
1983490
to
6acccc1
Compare
…m.IO.Pipelines, System.Text.Json and System.Net.Http.Json to minimum versions necessary Bump Erlang version, add comment
8157a4e
to
94bcfe7
Compare
Thank you @thompson-tomo! |
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 applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creatingthe 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.
CONTRIBUTING.md
documentFurther Comments
Closes: #1480