chore: update Polyfill from 1.32.0 to 9.8.1#4879
Conversation
|
Potential fixes for making the namespace
Update: |
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4879 +/- ##
==========================================
- Coverage 73.86% 73.31% -0.55%
==========================================
Files 496 482 -14
Lines 17951 17551 -400
Branches 3516 3426 -90
==========================================
- Hits 13259 12868 -391
+ Misses 3833 3824 -9
Partials 859 859 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| #if !NET7_0_OR_GREATER | ||
| public static IReadOnlyDictionary<TKey, TValue> AsReadOnly<TKey, TValue>(this IDictionary<TKey, TValue> dictionary) | ||
| where TKey : notnull => | ||
| new ReadOnlyDictionary<TKey, TValue>(dictionary); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
note: now included via Polyfill
| <PackageReference Include="Polyfill" Version="9.8.1" PrivateAssets="all" /> | ||
| </ItemGroup> | ||
| <PropertyGroup> | ||
| <PolyStringInterpolation>true</PolyStringInterpolation> |
There was a problem hiding this comment.
note: PolyStringInterpolation emits a DefaultInterpolatedStringHandler, which the compiler considers for String Interpolation when not all holes are strings. This is more efficient than String.Format. But makes our netstandard2.1, netstandard2.0 and net462 targets, and with that the entire Sentry NuGet package, a bit larger:
| <!-- the latest TargetFramework requires no Polyfills; "Sentry.Compiler.Extensions" has its own Polyfills; "Android" and "Cocoa" projects are referenced by "Sentry" --> | ||
| <Using Include="Polyfills" Condition="'$(TargetFramework)' != $(LatestTfm) And $(MSBuildProjectName) != 'Sentry.Compiler.Extensions' And $(MSBuildProjectName) != 'Sentry.Android.AssemblyReader' And $(MSBuildProjectName) != 'Sentry.Bindings.Android' And $(MSBuildProjectName) != 'Sentry.Bindings.Cocoa'" /> | ||
| </ItemGroup> | ||
|
|
There was a problem hiding this comment.
note: this (and see also test/Directory.Build.props) is required
because some of the Polyfills have been moved into the Polyfills namespace.
This needs to be conditional,
because Sentry, to which Polyfill is emitting the Polyfills to as internal types,
does not give access of it's internals to every project in the src (and test) directory.
Alternatively,
we could add the global using (<Using Include="Polyfills" />) to every project that needs it,
but I found that to be quite a lot of additions, and new projects don't have them then per default,
so I suggest here to do it in a more central place.
| --> | ||
| <ItemGroup> | ||
| <PackageReference Include="Polyfill" Version="1.32.0" PrivateAssets="all" /> | ||
| <PackageReference Include="Polyfill" Version="9.8.1" PrivateAssets="all" /> |
There was a problem hiding this comment.
issue: Test failure
- Project:
Sentry.Tests - Test:
Sentry.Tests.SentrySdkCrashTests.CauseCrashInSeparateProcess - TFM:
net48
Unhandled Exception: System.IO.FileLoadException: Could not load file or assembly 'System.Memory, Version=4.0.1.2, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
at Sentry.SentrySdk.CauseCrash(CrashType crashType)
at Sentry.Testing.CrashableApp.Program.Main(String[] args) in D:\GitHub\sentry-dotnet\test\Sentry.Testing.CrashableApp\Program.cs:line 9
| <!-- Due to Polyfill, System.Memory needs to be a top-level dependency, otherwise app start may fail with: | ||
| System.IO.FileLoadException: Could not load file or assembly 'System.Memory' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. --> | ||
| <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0' Or '$(TargetFramework)' == 'net462'"> | ||
| <PackageReference Include="System.Memory" Version="4.6.3" /> | ||
| </ItemGroup> | ||
|
|
There was a problem hiding this comment.
thought: This could be an issue!
related to https://github.com/getsentry/sentry-dotnet/pull/4879/changes#r2895293577
(Sentry.Tests.SentrySdkCrashTests.CauseCrashInSeparateProcess(CrashType) is a great test !!!)
I wonder if this may be considered a breaking change,
and if we should hold off on this upgrade for our next Major (Sentry v7 supporting .NET 11).
But this would mean to similarly hold off on follow-up changes like
(although, actually, we could simply write our own ANE.ThrowIfNull(object?) internal static extension method for now, and later replace it with the Polyfill-variant)
There was a problem hiding this comment.
Yeah I'd say we avoid adding dependencies to the core package... especially when it's just for polyfills which we don't need for any functional reason (they're just to make the code slightly more attractive to read really).
What's the most recent version of Polyfill that doesn't require we change any dependencies for SDK users? We could try bumping to that instead maybe?
There was a problem hiding this comment.
Great idea!
Unfortunately, the Polyfill changelog is quite ... unavailable.
Will "binary search" on a Windows machine.
Description
Update Polyfill from
1.32.0to9.8.1.Related
Additional Changes
FeatureHttpfor .NET Standard 2.0 and .NET Standard 2.1FeatureMemoryis not used whenSystem.Memoryis in an incompatible versionNew Options
The new version of
Polyfillalso comes with "StringInterpolation" polyfills.DefaultInterpolatedStringHandlerfor string Interpolation ($"{}") which is generally more efficient compared toString.FormatorString.Concat(when not all elements arestrings)Follow-up
Enable and use "ArgumentExceptions" polyfills: