[Trimming] Put HttpMessageHandler type resolution behind feature switch#10744
[Trimming] Put HttpMessageHandler type resolution behind feature switch#10744
Conversation
… variable by default - Add XaHttpClientHandlerType feature switch to RuntimeFeature.cs (disabled by default) - Update GetHttpMessageHandler() to return AndroidMessageHandler directly when feature disabled - Add ILLink substitution for the new feature switch - Add RuntimeHostConfigurationOption for the feature switch - Remove default value for AndroidHttpClientHandlerType MSBuild property - Add XA1042 error for NativeAOT when AndroidHttpClientHandlerType is set - Add XA1043 deprecation warning for MonoVM/CoreCLR when AndroidHttpClientHandlerType is set - Add XA1044 warning when XA_HTTP_CLIENT_HANDLER_TYPE is found in environment files - Update tests to reflect new behavior - Add documentation for new error/warning codes Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>
| [FeatureSwitchDefinition ($"{FeatureSwitchPrefix}{nameof (XaHttpClientHandlerType)}")] | ||
| [FeatureGuard (typeof (RequiresUnreferencedCodeAttribute))] | ||
| internal static bool XaHttpClientHandlerType { get; } = | ||
| AppContext.TryGetSwitch ($"{FeatureSwitchPrefix}{nameof (XaHttpClientHandlerType)}", out bool isEnabled) ? isEnabled : XaHttpClientHandlerTypeEnabledByDefault; |
There was a problem hiding this comment.
@simonrozsival can we use the existing feature switch, and we don't need a new one?
We'd duplicate it in this file like we did StartupHookProvider.IsSupported.
There was a problem hiding this comment.
So to use $(AndroidHttpClientHandlerType), devs would have to set $(UseNativeHttpHandler) to false? The UseNativeHttpHandler switch should mean true -> AndroidMessageHandler, false -> SocketsHttpHandler. I would prefer to keep those feature switches separate.
There was a problem hiding this comment.
Ok, I see, so the problem is we have a property you can set the type name. This is really a different feature than UseNativeHttpHandler=true/false.
I would be in favor of considering removal, when I look at the projects using it, they are Xamarin.Android:
There was a problem hiding this comment.
So you would straight up remove it and give a warning, instead of allowing to still opt-into it via a feature switch?
There was a problem hiding this comment.
Yes, I think we should warn if you use it on release/10.0.1xx branch.
Then main we upgrade warning to an error, and just remove the feature.
AndroidHttpClientHandlerTypeandXA_HTTP_CLIENT_HANDLER_TYPErely on runtime type resolution viaType.GetType(), which is incompatible with trimming and NativeAOT. This PR disables this feature by default and adds deprecation warnings.Changes
Runtime
XaHttpClientHandlerTypefeature switch toRuntimeFeature.cs(disabled by default)GetHttpMessageHandler()returnsAndroidMessageHandlerdirectly—no reflection_AndroidUseXaHttpClientHandlerTypeBuild-time validation
AndroidHttpClientHandlerTypeset → build failsAndroidHttpClientHandlerTypeset → deprecation warningXA_HTTP_CLIENT_HANDLER_TYPEfound in@(AndroidEnvironment)filesProperty defaults
$(AndroidHttpClientHandlerType)inDefaultProperties.targetsMigration
Pass custom handlers via constructor instead of MSBuild property:
Documentation
Added docs for XA1042, XA1043, XA1044.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.