SimonCropp / Polyfill

Source only package that exposes newer .net and C# features to older runtimes.
MIT License
275 stars 19 forks source link

considere packing the targets as buildTransitive too #237

Open Doraku opened 1 week ago

Doraku commented 1 week ago

First let me preface this by thanking you for this awesome work. I was somewhat doing a much poorer implementation of your solution before I found your package, deleting all that ugly stuff was a relief ❤️

Is the feature request related to a problem

I'm making a package to ease up my configuration across for all my project, much like your ProjectDefaults and I want to include a reference to Polyfill. While this project is a development dependency that is not supposed to flow as an indirect reference, it is actually possible to include it by removing the PrivateAssets="all" of the PackageReference. I'm doing this to automatically include a collection of analyzer packages for exemple.

Describe the solution

The issue is that the targets file of Polyfill does not flow as an indirect dependencies because it's not present in a buildTransitive folder of the nuget (more info here) just in case). Doing so, while kind of unnatural for a development dependency (hence why I am requesting your opinion here), is quite an easy fix by adding <file src="Polyfill.targets" target="buildTransitive"/> to the nuspec.

Describe alternatives considered

Currently I am repacking your code files into my own package like so

  <ItemGroup>

    <PackageReference Include="Polyfill" Version="7.0.0" PrivateAssets="all" GeneratePathProperty="true" />

    <!-- fix for Polyfill transitive reference -->
    <None Include="$(PkgPolyfill)\build\Polyfill.targets" Pack="true" PackagePath="projects" Visible="false" />
    <Compile Update="$(PkgPolyfill)\contentFiles\**\*" Pack="true" PackagePath="contentFiles\cs\netstandard2.0\Polyfill\" Visible="false" />

  </ItemGroup>

and in the targets file of my own package

  <!-- fix for Polyfill transitive reference -->
  <Import Project="$(MSBuildThisFileDirectory)Polyfill.targets" Condition="Exists('$(MSBuildThisFileDirectory)Polyfill.targets')"/>
  <ItemGroup>
    <Compile Update="@(Compile)">
      <Visible Condition="'%(NuGetItemType)' == 'Compile' and '%(NuGetPackageId)' == 'DefaultCSharp'">false</Visible>
    </Compile>
  </ItemGroup>

This works fine but obviously I would prefer for your project to be a proper dependency instead of hiding it like so, more people who are stuck supporting multiple targets should know about it :).

Additional context

My full project is here for completeness, again thank you for this.

SimonCropp commented 1 day ago

wouldnt this change the default behavior of polyfill? ie that we dont want to to flow through dependencies?

Doraku commented 1 day ago

Since your package is declared as a development dependency it should be added by default with PrivateAssets="All" which prevent its transitive inclusion. You have to explicitly remove that for the reference to flow, which nobody should do (except me because I like going against the flow).

Again my usage is definitely not conventional so I would totally understand you not making this change.