Skip to content
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

DO NOT MERGE: spike demonstrating viabilty of removing UWP code from System.Reactive #3

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

idg10
Copy link
Owner

@idg10 idg10 commented Feb 23, 2023

This is an experiment to demonstrate the viability of removing all UWP Code from the System.Reactive component.

The main factor motivating the existence of a UWP-specific target in System.Reactive was that UWP used to have some significant threading limitations. Any attempt to produce a 'lowest common denominator' implementation of certain scheduler functionality would result in suboptimal behaviour on other platforms.

However, starting with Windows 1709 (the 2017 'Fall Creators Update' edition), and the corresponding version 10.0.16299 of the Windows SDK, UWP provided full support for .NET Standard 2.0. This means that the schedulers used on other platforms now work perfectly well on UWP, so there's no longer much of a reason to offer a UWP-specific build.

The one issue is that the ThreadPoolScheduler in the UWP build of System.Reactive had some UWP-specific surface area. (You can specify the WorkItemPriority and WorkItemOptions it should use.) This spike addresses that by adding a new WindowsThreadPoolScheduler in the System.Reactive.Windows.Threading project that provides this UWP-specific functionality. UWP applications will get the normal ThreadPoolScheduler implementation because they are using the same .NET Standard 2.0 build as everything else, but if they need the UWP-specific thread pool handling, they can still get it. (It's a breaking change because it requires a reference to the System.Reactive.Windows.Threading package, and the name of the type has changed, but the functionality remains available.)

The motivation for showing that we can move the UWP code out of the main System.Reactive component is that UWP tooling has fallen behind. Microsoft discourages the use of UWP, pushing people to use WinUI instead. That's not always possible—quite a lot of Windows functionality available in UWP is not yet available in WinUI so Rx needs to continue to support UWP. But we don't want it to be a major point of friction for all other Rx development. The lack of support for UWP across great swathes of .NET tooling means that it is a point of friction, so we wanted to see if it was practical to separate the UWP bits out into their own components.

This PR shows that this does seem to be possible.

Here are the specific changes made on this branch:

  • All target framework monikers have been updated to frameworks that are currently in support (so .netcoreapp3.1 has gone, net5.0 has been replaced with net6.0, and the projects that still target UWP now specify 10.0.18362, the oldest version that VS2022 officially supports, instead of the 10.0.16299 that Rx v5 uses)
  • The main test project now targets net6.0 and .net7.0 (and I've removed the netcoreapp2.1, netcoreapp3.1, and net5.0 targets)
  • We're now using MSTest instead of xUnit (because MSTest supports UWP; xUnit's UWP support is broken on VS 2022)
  • UWP-specific code has been moved into either System.Reactive.WindowsRuntime or System.Reactive.Windows.Threading (which in almost all cases just meant putting classes back in the components they used to live in back in v3; most of the types in question had type forwarders in these assemblies, so they've essentially just returned home)
  • All projects that had a UWP TFM (uap10.0.16299) have had that removed with the exception of the two that do actually contain some UWP code, System.Reactive.WindowsRuntime and System.Reactive.Windows.Threading
  • I've complete removed MSBuild.Sdk.Extras; because of the changes above, most projects no longer had any need for it; as for the two that are still using UWP, I was unable to find a combination of project file contents that would produce a clean build and which would open without error in VS, so I've ended up doing things the hard way in those two projects, which has the handy side effect that we get to remove the dependency on MSBuild.Sdk.Extras

`netcoreapp3.1` removed
`net5.0` -> `net6.0`
`net5.0-windows10.0.19041` -> `net6.0-windows10.0.19041`

Tests.System.Reactive additionally targets net7.0 to verify that everything works there too.

Also removed System.Runtime.Interop.WindowsRuntime NuGet package reference for .NET Standard build of System.Reactive because it didn't work properly with C#/WinRT anyway, and removing it enables us to remove one of the things forcing us to produce more targets than we'd like.
xUnit doesn't seem to support UWP any more in practice, so this change tries moving everything over to MSTest instead, because it continues to work on VS 2022 with UWP code.

Although this is a fairly mechanical translation (we're not using anything in xUnit's API that doesn't have direct equivalents in other test frameworks), this has uncovered one issue. Unlike xUnit, MSTest doesn't supply a SynchronizationContext, which causes several of the tests to execute difference code paths. Some of them fail as a result.

It's unclear whether this has uncovered latent bugs in Rx, or whether the absence of a synchronization context effectively makes these tests invalid. For now, since this is just an experiment, I've disabled the offending tests.
UWP is a source of many problems in the tooling, and the single biggest reason for including a UWP-specific version in the main `System.Reactive` package was that UWP used not to have full support for threads, necessitating a special UWP-flavoured ThreadPoolScheduler.

But as of Windows 10.0.16299, UWP provided full .NET Standard 2.0 support, so there's no longer any strong reason to have a UWP-specific build. (Annoyingly, some UWP-specific surface area sneaked in, which this resolves by moving that into a new WindowsThreadPoolScheduler in the System.Reactive.WindowsRuntime project. This is a breaking change for UWP clients relying on how ThreadPoolScheduler looked in v5.0, but it only requires a reference to this other package, and to change to using WindowsThreadPoolScheduler.)

All UWP-specific surface area is now in a separate library. Most of it has simply moved back where it used to be - type converters for most of the types were in place, so they've just come back home.

This commit a few project changes that are not fundamental to the goal, but which seemed to be necessary to work around problems arising from limited support for UWP in the tooling. For example, I ended up having to remove the use of MSBuild.Sdk.Extras from some projects.
Since this branch is aiming to confine UWP to the handful of projects that need it (two, it appears) and since the remaining ones ended up having to move away from MSBuild.Sdk.Extras anyway, nothing in the solution needs it any more.
@idg10 idg10 self-assigned this Feb 23, 2023
@idg10
Copy link
Owner Author

idg10 commented Feb 23, 2023

Note: I'm not sure we'd necessarily go with this exactly transformation. This was the "change the smallest number of things to prove the point" approach, and it entailed reverting a couple of the existing façade libraries back to their original purpose.

The problem with those libraries is that they split up types in a slightly weird way. There's a System.Reactive.WindowsRuntime library, but only some of the WinRT things end up in there. Some WinRT-specific code ends up in System.Reactive.Windows.Threading (which has happened because when I moved types out of System.Reactive I restored them to their earlier homes).

I think the reason the UWP-specific CoreDispatcherObservable and CoreDispatcherScheduler ended up in a library that wasn't obviously UWP-specific is that over the years, several UI frameworks have had a degree of source-level compatibility—in particular the Dispatcher concept existed in WPF, Silverlight, several editions of Windows Phone, Metro, and UWP. I've not done the necessary archaeology to conform this, but I think it might be that part of the idea behind System.Reactive.Windows.Threading is that regardless of which of these frameworks you were using, you'd just add a reference to that, and if would give you Dispatcher-aware types that were appropriate for whichever particular UI framework you were targeting. (But I think at some point, the source-level quasi-compatibility that this provided got dropped when the word Core was prepended to some types. So it's unclear to me whether this particular packaging concept still offers any value.)

We are considering attempting to separate out UI-framework-specific Rx support into completely separate repos. (That would be in line with enabling other UI frameworks such as Avalonia to provide their own integration if they were minded to do so.) So the change in this PR would be a stepping stone to having completely separate repos for Rx's UWP, WPF, and Windows Forms support. (And perhaps one for WinUI too; or at any rate, the advent of C#/WinRT means that the UWP-flavoured integration with WinRT foundation types is based on a projection that is deprecated—it works differently in .NET 6.0 and up.)

There's some additional work to be done to make that possible (especially around testing) so the goal with this PR was just to show that it is at least possible to have a System.Reactive component that works fine on UWP without supplying a UWP-specific target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant