-
Notifications
You must be signed in to change notification settings - Fork 782
Rx 7.0 packaging changes #2268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rx 7.0 packaging changes #2268
Conversation
Deprecate all UI-framework-specific and platform-specific types in System.Reactive. Add various System.Reactive.Integration.... packages with the replacement types.
It looks like a few tests that fetched DispatcherScheduler.Current used to rely on the Dispatcher already having been created. Up until recently, that was true by random chance. But recent changes, which presumably have affected the order of test execution, mean that's no longer true. These tests should always have ensured a dispatcher was available before starting to run, and now they do.
* Add Obsolete attributes * Add the newly-public (was internal) member of AsyncLock
Change SDK file ref to a version available on the build agent Replace net6.0 with net8.0 Remove uap10.0.18362 from non-legacy components Use .NET SDK PackageValidation for System.Reactive API compatibility check. Initial ADR drafts Use Microsoft.CodeAnalysis.PublicApiAnalyzers in System.Reactive.Net Remove the ApiApprovalTests that are now superceded by the use of package validation for the legacy System.Reactive package, and public API analyzers for the new System.Reactive.Net.
Before, the build system didn't know we are using a custom ref assembly. This meant, amongst other things, that tests no longer built.
This was after all the entire point of this branch.
These crept in from a merge.
We have now decided not to use that naming convention, and instead to reinstate some of the old packages that had been turned into facades.
mwadams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need a comment somewhere to the effect that leaving these types in the boundaries will not increase the size of size-sensitive deliverables because they will get trimmed?
Good stuff.
Rx.NET/Documentation/adr/0006-uwp-legacy-threadpoolscheduler-in-facade.md
Outdated
Show resolved
Hide resolved
Rx.NET/Documentation/adr/0006-uwp-legacy-threadpoolscheduler-in-facade.md
Outdated
Show resolved
Hide resolved
Rx.NET/Documentation/adr/0007-api-compatibility-verification.md
Outdated
Show resolved
Hide resolved
Rx.NET/Documentation/adr/0006-uwp-legacy-threadpoolscheduler-in-facade.md
Show resolved
Hide resolved
Rx.NET/Source/src/System.Reactive.WindowsRuntime/System.Reactive.WindowsRuntime.csproj
Outdated
Show resolved
Hide resolved
Rx.NET/Source/src/System.Reactive/Platforms/WinRT/Concurrency/ThreadPoolScheduler.Windows.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades Rx.NET from version 6.1.0 to 7.0.0, implementing a major architectural change by extracting UI framework-specific functionality into separate packages while maintaining binary compatibility with Rx 6.0.1.
Key changes:
- Version bump from 6.1.0-preview to 7.0.0-preview
- Extraction of WPF, Windows Forms, and Windows Runtime support into separate packages (
System.Reactive.Wpf,System.Reactive.Windows.Forms,System.Reactive.WindowsRuntime) - Migration from
HAS_DISPATCHERtoHAS_WPFconditional compilation symbols throughout test files - Updates to build pipeline to handle reference assemblies and package generation
- API compatibility validation configuration added to maintain backwards compatibility
Reviewed Changes
Copilot reviewed 121 out of 142 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| azure-pipelines.rx.yml | Removed NuGet pack task for compatibility package; added cleanup of MakeRefAssemblies artifacts |
| Rx.NET/Source/version.json | Version bumped to 7.0.0-preview |
| Multiple test files | Replaced HAS_DISPATCHER with HAS_WPF conditional compilation symbol |
| System.Reactive.csproj | Added API compatibility validation, custom nuspec handling, and reference assembly support |
| System.Reactive.Wpf/* | New WPF support package with extracted WPF-specific functionality |
| System.Reactive.WindowsRuntime/* | New Windows Runtime support package with extracted WinRT-specific functionality |
| System.Reactive.Windows.Forms/* | New Windows Forms support package with extracted WinForms-specific functionality |
Files not reviewed (6)
- Rx.NET/Source/src/System.Reactive.Analyzers/Resources.Designer.cs: Language not supported
- Rx.NET/Source/src/System.Reactive.WindowsRuntime/Strings_WindowsThreading.Designer.cs: Language not supported
- Rx.NET/Source/src/System.Reactive.Wpf/Strings_Wpf_WindowsThreading.Designer.cs: Language not supported
- Rx.NET/Source/src/System.Reactive/Strings_Core.Designer.cs: Language not supported
- Rx.NET/Source/src/System.Reactive/Strings_Linq.Designer.cs: Language not supported
- Rx.NET/Source/src/System.Reactive/Strings_Providers.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
Rx.NET/Source/src/System.Reactive/Platforms/WinRT/Strings_PlatformServices.Designer.cs:42
- The resource name includes the full namespace path 'System.Reactive.Platforms.WinRT.Strings_PlatformServices', but the resx file is in Platforms/WinRT folder. Verify that the resource manager can correctly locate this resource file at runtime, as the embedded resource name may not match this path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rx.NET/Source/src/System.Reactive/Internal/HostLifecycleService.cs
Outdated
Show resolved
Hide resolved
Rx.NET/Source/src/System.Reactive/Internal/HostLifecycleService.cs
Outdated
Show resolved
Hide resolved
Rx.NET/Source/src/System.Reactive.Wpf/System.Reactive.Linq/DispatcherObservable.cs
Outdated
Show resolved
Hide resolved
...Source/src/System.Reactive.Analyzers.Test/Verifiers/AddUiFrameworkPackageAnalyzerVerifier.cs
Show resolved
Hide resolved
...Source/src/System.Reactive.Analyzers.Test/Verifiers/AddUiFrameworkPackageAnalyzerVerifier.cs
Show resolved
Hide resolved
...urce/src/System.Reactive.Analyzers/Analyzers/UiFrameworkPackages/UiFrameworkSpecificTypes.cs
Show resolved
Hide resolved
|
Tiny nit: The "Tags" of the nuspec is |
Resolves #1461
Resolves #1745
This moves all of the UI-framework-specific types out of the public API of
System.Reactive, and into framework-specific packages:System.Reactive.Windows.FormsSystem.Reactive.WpfSystem.Reactive.WindowsRuntimeFor backwards compatibility purposes, these types remain available at runtime; they have been removed only from the assemblies in the
reffolder of theSystem.ReactiveNuGet package. Code that was built against Rx 6 or older will continue to run. If it is rebuilt against Rx 7 and attempts to use UI-framework-specific features, compiler errors will result, but this change also adds an analyzer that detects when this happens, and recommends adding the relevant package.As #2246 proposes, this also removes the old facade packages that haven't changed in an age. (We will also need to mark these as deprecated on NuGet itself at some point.) Note that
System.Reactive.Windows.FormsandSystem.Reactive.WindowsRuntimewere, for many years, facades. This reinstates them as proper packages. (TheSystem.Reactive.Wpfpackage is new because the existing facade that refers to WPF types has a confusing name due to some long-since irrelevant history, so resurrecting that package did not seem like a good idea..)