manuelbl / SwissQRBill.NET

.NET library for Swiss QR bill payment slips (aka QR-Rechnung)
MIT License
86 stars 32 forks source link

Remove dependency on the System.Text.Encoding.CodePages NuGet package #33

Closed 0xced closed 2 years ago

0xced commented 3 years ago

CodePagesEncodingProvider.Instance is not part of .NET Standard 2.0 but is available (built-in) on all supported versions of .NET Core, see https://apisof.net/catalog/653e5d69-d3b8-cf7c-2875-7f1822ab1354

So we register the CodePagesEncodingProvider.Instance through reflexion in order to avoid taking a superfluous dependency on the System.Text.Encoding.CodePages NuGet package.

On .NET Framework, this issue is moot since Encoding.GetEncoding(1252) always succeeds.

manuelbl commented 3 years ago

Thanks for the PR. I'll have a detailed look at it.

Is this more of a minor clean up? Or are there cases where the additional dependencies causes issues?

0xced commented 3 years ago

Is this more of a minor clean up? Or are there cases where the additional dependencies causes issues?

I have no particular issue except that having this dependency weights an additional ~750 KB. For the app that I build which is only a few MB those 750 KB are not negligible.

I'm currently using this trick to exclude System.Text.Encoding.CodePages.dll from the final exe but it would be nice to not need this workaround at all:

<Target Name="RemoveUnneededDependencies" AfterTargets="ResolveLockFileCopyLocalFiles">
  <ItemGroup>
    <!--
      System.Text.Encoding.CodePages is implicitly referenced by Codecrete.SwissQRBill.Generator and is not needed on .NET Framework.
      As we don't need it we can save 744 KB from the final exe by excluding it.
      See also https://github.com/manuelbl/SwissQRBill.NET/pull/33
    -->
    <ReferenceCopyLocalPaths Remove="@(ReferenceCopyLocalPaths)" Condition="$([MSBuild]::ValueOrDefault('%(NuGetPackageId)', '').Equals('System.Text.Encoding.CodePages'))" />
  </ItemGroup>
</Target>
0xced commented 3 years ago

And if someone tries to generate QR-bill PDFs on .NET Core 2.0 or 2.2 (where System.Text.Encoding.CodePages is not built-in) then they would get the following exception:

System.NotSupportedException No data is available for encoding 1252. For information on defining a custom encoding, see the documentation for the Encoding.RegisterProvider method.

And then they would be able to fix this issue at the application level by taking a dependency on System.Text.Encoding.CodePages and calling Encoding.RegisterProvider(CodePagesEncodingProvider.Instance) at application startup, before using SwissQRBill.NET.

But who uses .NET Core 2.0 or 2.2 which are both out of support anyway? 🤷

manuelbl commented 3 years ago

I will merge this PR with the next major release as it is a breaking change. At the same time, I will likely drop support for .NET Core 2.x. There are no specific plans yet when this will be.

0xced commented 3 years ago

If you keep supporting .NET Standard 2.0 (which I would strongly advise) then .NET Core 2.0 and later are automatically supported.

And while we are on the topic of breaking changes, don't forget about #22 for the next major release. 😉

manuelbl commented 2 years ago

Merged to branch "v3"

0xced commented 2 years ago

Closing since it was merged in commit 2c4a123c8b79da5b88bdb1d3b81ba086ce8d9b74.