Repository navigation
chore: update Polyfill from 1.33.2 to 11.4.1 - #5670
jamescrosswell wants to merge 3 commits into
Conversation
Polyfill is source-only and declares no NuGet dependencies, so the upgrade leaves Sentry's package dependencies unchanged. Unlike #4879, this keeps PolyStringInterpolation off and adds no System.Memory reference. - Remove our AsReadOnly(IDictionary) polyfill, now provided by Polyfill - Import the Polyfills namespace in projects with access to Sentry internals on non-.NET Core targets - Suppress PolyfillMemoryVersion in the Roslyn components, which only have System.Memory 4.5.4 Refs #5006 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Trade-off: assembly size Polyfill compiles its whole API surface into the assembly as internal code. There's no option to include only the polyfills we actually use. Compared with the published Sentry 6.12.0 (Release builds):
Apps that trim, such as mobile and Native AOT apps, should drop most of the unused polyfill code. Desktop and .NET Framework apps ship all of it. What we get for it: one fewer hand-written polyfill today, plus access to newer polyfilled APIs, e.g. the @ric-oliv Is roughly 290 KB on the .NET Framework and netstandard targets, and 70–160 KB on net8–10, an acceptable price for that? If not, staying on 1.33.2 costs us nothing functionally. |
Polyfill 11.4.1's net9.0 ConditionalWeakTable.Remove(key, out value) lacks the DynamicallyAccessedMembers(PublicParameterlessConstructor) annotation on TValue that ConditionalWeakTable<TKey, TValue> declares, so Native AOT trim analysis of Sentry.TrimTest fails with IL2091. Sentry doesn't call it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
40% more size for no return... I'd rather not bring it in at this point 🤔 And my agent also has some concerns about how it will behave on older integrations such as netstandard2.0 and net462, that it can crash in specific scenarios. Probably not worth the effort right now? |
|
Won't do - increased size and limited benefit. |
Summary
Updates Polyfill from 1.33.2 to 11.4.1 without changing Sentry's dependencies.
#4879 tried this upgrade but added
System.Memory4.6.3 as a top-level dependency fornet462andnetstandard2.0, which is why #5006 was parked for the next major. Polyfill didn't need that dependency; it's a source-only package with no NuGet dependencies of its own. It was a workaround forSentrySdkCrashTests.CauseCrashInSeparateProcessfailing on net48 oncePolyStringInterpolationwas enabled:PolyStringInterpolation,SentrySdk.CauseCrashhas to loadSystem.Memorybefore it crashes.CrashableApp.exefrom the Sentry.Tests output folder. That folder has System.Memory 4.6.3 (assembly 4.0.5.0), whileSentry.dllis compiled against 4.0.1.2 via System.Text.Json 8.0.5, and nothing redirects between the two.This PR leaves
PolyStringInterpolationoff.Changes
Sentry,Sentry.AnalyzersandSentry.Compiler.Extensions.AsReadOnly(IDictionary)polyfill. Polyfill now ships the same method, so the calls became ambiguous.using Polyfills;for non-.NET Core targets insrc/andtest/Directory.Build.props. Polyfill moved its extension methods into that namespace, and projects with access to Sentry's internals (DiagnosticSource, Serilog, OpenTelemetry, Sentry.Testing) use them.PolyfillNoWarnIncorrectVersionin the two Roslyn components. They only get System.Memory 4.5.4 from Roslyn, so Polyfill leaves out its span polyfills there and warns about it.ConditionalWeakTable.Remove(key, out value)polyfill. On net9.0 itsTValuelacks theDynamicallyAccessedMembersannotation thatConditionalWeakTablerequires, which fails Native AOT trim analysis with IL2091, and we don't use it. This is Polyfill's bug, still present on its main branch.Compatibility
Compared with
main:Sentry.dll's assembly references are identical on netstandard2.0 and 2.1. On net462 it also referencesSystem.Numerics,System.XmlandSystem.Xml.Linq, which ship with .NET Framework itself, not as NuGet packages.So this shouldn't need to wait for the next major. The trade-off is assembly size; see the comment below.
Notes
Polyfillsnamespace, and could hit the same ambiguity or missing-using errors. I haven't checked this.Closes #5006
🤖 Generated with Claude Code