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! experimental split of UI-framework-specific parts out of main packages #2034

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

Conversation

idg10
Copy link
Collaborator

@idg10 idg10 commented Nov 7, 2023

This is an experiment showing one way Rx could solve the problems described in AvaloniaUI/Avalonia#9549

With these changes, programs can take a dependency on a newly-introduced System.Reactive.Base component. (Not currently available on NuGet—only in this experimental branch.) This is effectively exactly the same API surface area as you got with System.Reactive. The difference is that it won't bring in references to WPF or Windows Forms just because you happen to target a net6.0-windows.10.0.19041 (or later) TFM.

System.Reactive still exists (and still does the same thing to you) but it's now just a backcompat facade. This essentially moves all of System.Reactive into the new System.Reactive.Base, except for the bits that were causing problems.

Note that there is one breaking change: if you target UWP, System.Reactive now gives you the same ThreadPoolScheduler as all the other platforms get. If you were relying on the UWP-specific features, you need to switch to the new WindowsRuntimeThreadPoolScheduler, which provides the exact same functionality that used to be available on the old ThreadPoolScheduler. This was the only way we could fully push all of the UI-specific functionality out of the core of Rx and into UI-framework-specific components. (In all other cases we could just relocate whole types. This was the one case where a type available in the netstandard2.0 and net6.0 targets also existed in another target but with a different API.) Since use of UWP has been discouraged for some time, and because Rx 5.0 and 6.0 continue to work just fine, our view is that this particular breaking change is tolerable, particularly given the problems it solves for everyone else.

@idg10 idg10 marked this pull request as draft November 7, 2023 18:54
@glopesdev
Copy link

glopesdev commented Nov 8, 2023

@idg10 this looks great, thanks for starting this!

I have to say I am a bit concerned with the decision to create a new package System.Reactive.Base.

The current package ecosystem is already very confusing, having suffered several breaking changes in the past including package renaming, package splitting, package re-unifying, changing strong name signatures, etc. I think this was ultimately damaging to the Rx.NET community. I agree we really need to clean-up, but somehow introducing yet another new package into the mix just feels like it will exacerbate the problem even further. There is an existing System.Reactive.Core package which follows the original distribution strategy for Rx and which exposes almost the exact same surface area as this new System.Reactive.Base.

I would usually be the first to defend full backwards-compatibility, but unfortunately the current state of Rx.NET packages is completely untenable. Continuing to maintain a meta-package System.Reactive tied to an arbitrary set of UI platforms will send the wrong signal and just by inertia cause confusion. I would rather we just strip System.Reactive out of all the UI-platform-specific functionality (which really shouldn't be there to begin with). If the change is going to be breaking anyway, my feeling is we should take this opportunity to do a proper clean up.

As proposed in #1847 we would then introduce new platform-specific packages, e.g. System.Reactive.WinForms, System.Reactive.Wpf, System.Reactive.Maui, etc. which would make it much easier for developers to move with the constantly inevitable changes to UI technology stack.

I understand the concerns with backwards compatibility, but legacy apps can always lock their package versions to an earlier major release of the package. If the app is not legacy, then they can usually update in a few minutes since the API surface area is exactly the same except for a few edge cases. Indeed making it break by stripping System.Reactive will make it much more obvious which exact points in the code they will have to change.

@idg10
Copy link
Collaborator Author

idg10 commented Nov 8, 2023

I have to say I am a bit concerned with the decision to create a new package System.Reactive.Base.

The current package ecosystem is already very confusing

I would very much prefer not to introduce a new package. If I can possibly find an acceptable way not to do it, then I will avoid it. So I agree with your concern. However...

Actually before I start discussing this, I've realised that I left out one quite important file from this draft PR, because it was on a different branch. I've just pushed f49ebda, so if you look now at this:

https://github.com/idg10/reactive/blob/experiment/typeforward-all-of-systemreactive/Rx.NET/Documentation/adr/0003-multi-platform-packaging.md

you'll see the various factors I'm aware of that make this challenging. That doc is a bit of a mess right now because it has changed a few times as I've tried various experiments, but it does at least collect together a lot of the issues that mean this isn't straightforward. I'm still working on it and am hoping to get it better resolved soon.

So... with this "let's unbreak Avalonia" project, my initial goal was to retain System.Reactive as the main package. (I.e., I wanted to do exactly what you want to do.) But I now think this isn't possible without breaking people relying on that bringing in the WPF/WinForms features. (And in some cases this would break them not in a "you have to update all your package references to get this working again" way, but in a "you will now be getting MissingMethodExceptions unless/until you can convince the authors of all the components you depend on that use Rx to upgrade to Rx 7.0. It's that "this is now broken in a way that's completely outside of people's control to fix" that I especially want to avoid. It was this kind of "there is actually no way for us to fix this without our dependencies fixing some things for us" problem that drove Avalonia to drop Rx.)

The fundamental problem occurs here if a single application has two dependencies (which might be indirect, non-obvious dependencies) of this form:

  • LibA uses System.Reactive v5.0, and depends on DispatcherScheduler
  • LibB uses System.Reactive v7.0 and uses a V7.0-specific feature (e.g., imagine for the sake of argument that we add a RateLimit operator to deal with the fact that almost nobody likes Throttle)

LibA will require DispatcherScheduler to be available through System.Reactive. So System.Reactive had either better contain that type, or contain a type forwarder pointing to wherever that type is now. If it doesn't LibA is going to fail at runtime with a MissingMethodException.

LibB will require Observable.RateLimit to exist. In this projected future in which System.Reactive is still the go-to package for Rx, LibB will require Observable to be available through System.Reactive. So System.Reactive had either better contain that type, or contain a type forwarder point to wherever Observable is now. Otherwise, LibB will fail at runtime with a MissingMethodException.

Now think about this proposed future in which Rx 7 has a System.Reactive which does not contain any UI-specific features. The app using LibA and LibB has to pick a single version of Rx. It can't pick v5 or v6 because Observable.RateLimit won't exist in those versions, so LibB will fail with a MissingMethodException, so we have to be using System.Reactive v7.

But what does that mean for LibA? It will expect System.Reactive either to contain DispatcherScheduler, or to contain a type forwarder saying where to find DispatcherScheduler. Well it can't contain it, because we've decided that we're removing all UI bits from System.Reactive.

So can System.Reactive still contain a type forwarder for DispatcherScheduler? If it does, then it's going to acquire a dependency on whatever component that's defined in—let's say it's System.Reactive.Integration.Wpf. (At least, the mechanism the the C# compiler supplies for adding type forwarders requires you to have a DLL available that defines the real thing.) But that's a component that exists only in .net*.0-windows10.* TFMs. So that means we'd have a Windows-specific target in System.Reactive with a dependency on a component with a dependency on WPF...which is exactly the problem we're trying to avoid. The problem would have moved out to a dependency, but it'd be a non-optional dependency, so we've merely moved the problem, not actually solved it. Anyone having problems today will continue to have problems.

If you can think of some way that System.Reactive could have type forwarders for DispatcherScheduler (and all the other problematic types) that connect up with the real implementations when running in an environment where they're actually available, but which don't really point anywhere real otherwise, then perhaps we could make it work. But it's not clear to me that this is possible. And even if it is possible, we still end up with a situation in which the main Rx package has a public API surface area that includes, for perpetuity, UI-framework-specific types (possibly supported by some unorthodox hacks to enable us to build in type forwarders without also getting dependencies on packages we really don't want). This is not my idea of a clean break.

The only way to make it proper clean break is to declare that anyone in the LibA+LibB scenario described is out of luck. But a problem with that is that it gives library authors a strong incentive not to upgrade to Rx 7 just in case they end up being the root cause of that kind of problem. So that also doesn't really feel like a clean break—we're still dealing with the legacy of the problem. This was the appeal of introducing a whole new component: that really can be a clean break.

So at this point I am pretty sure that System.Reactive is, unfortunately, doomed to be a backcompat facade. This makes me very sad.

I did consider resurrecting System.Reactive.Core, just so we weren't increasing the number of libraries by quite so much. In fact, that's what the ADR says we will do, although it's not what this experiment actually does right now. And I've not actually ruled it out, but having looked at the download stats on NuGet, I think there's a high risk of causing problems for quite a lot of people. It would be tempting to think of this component's role as a backcompat facade as ancient history. However, we get about 170,000 downloads per week of the V4 era System.Reactive.Core component, and V4 was, I believe, the point at which System.Reactive.Core first got relegated to a backcompat facade.

That's a lot of downloads. Apparently people still have systems that were written in an era where it definitely mattered that these particular components work the way they do. There are a lot of people with legacy systems frozen in time for one reason or another. (In the last few months I've helped a customer with a project where they just migrated off VB6!) So we do need to bear in mind what might happen to someone who is still depending on a thing that was done in Rx v4 if they end up having an upgrade to Rx v7 forced on them by some dependency they can't control.

There's one particular aspect of System.Reactive.Core that concerns me. That component (and in fact everything in the facades folder) currently has a non-obvious feature: these components retain the slightly odd version numbering scheme that was introduced as the first (rather disastrous) attempt to solve the problems in which a VS plug-in loading the net40 version of Rx would clobber any plug-in that subsequently tried to load the net45 version. Rx v3 introduced a versioning system where they put slightly different version numbers (3.0.1000.0, 3.0.2000.0, and so on) into the different targets of Rx. This meant it became possible to load different targets of the "same" component into the same .NET FX process because they had different strong names.

If System.Reactive.Core once again becomes the main Rx package, I think we'd want its assembly version numbers to be 7.0.something. And we'd be changing the targets it offers: most likely net6.0 so we can offer trimming support, and netstandard2.0 so we can support all the not-.NET-6.0-or-later targets we want to support.

That's a radical change for a component which for the last 7 years has been a backcompat package in which the version numbers have been a) fixed, and b) weird in specific ways to solve particular problems. My worry is that by changing what this component is, legacy systems that are still today relying on how this weird version numbering fixes a problem for certain plug-in architectures will find that they are in trouble if something forces Rx onto v7.

My real worry here is that I have absolutely no idea what the consequences of reinstating System.Reactive.Core as the main Rx package would be for these older projects that are responsible for 170,000 downloads per week. Today, if something forces them on to Rx 6, it shouldn't be a problem because these backcompat facades are exactly what they have always been. But if we reinstate System.Reactive.Core as a main component in Rx 7, that will no longer be true.

If the change is going to be breaking anyway, my feeling is we should take this opportunity to do a proper clean up.

As it currently stands, we will break UWP backcompat. But nothing else. I'm a lot more comfortable with breaking UWP backcompat than I am with breaking .NET FX backcompat. (Also, the gains from ditching UWP are huge: its presence causes us an unbelievable amount of grief.) The number of downloads of the UWP-specific Rx packages are 1,000x lower than the numbers for the backcompat shims in Rx v4.

So I don't currently agree with the idea that because there's a breaking change, we can break much more. The numbers suggest that UWP is used so much less that the impact of a breaking change there is acceptable.

the current state of Rx.NET packages is completely untenable.

I wholeheartedly agree with this.

Your proposal—breaking backwards compatibility for the main Rx package—is radical. So I think you agree that radical change is required.

This is why I'm prepared to contemplate yet another change to "Will the real Rx package please stand up?" much as I don't want to do it. Unfortunately, while I massively prefer an end state in which System.Reactive simply doesn't have all the stuff it should never have had, I think there's no way for Rx consumers to work around the compatibility problems it will cause.

However, I would LOVE to be convinced otherwise. I really don't want to do what I currently think I'll have to do.

@mwadams
Copy link
Collaborator

mwadams commented Nov 8, 2023

To some extent this boils down to this debate

  1. Do we want to execute a change that fixes Avalonia (and any other need-for-slim-Rx customers) with minimal-to-no change required for the existing ecosystem. Make that vNext and designate it as the LTS version for the existing ecosystems and declare an intention for radical change in the roadmap (e.g. netcore only, radically simplified packaging a la AspNet) This will be gnarly but minimize impact to address the specific issue on the current roadmap.

  2. Or do we want to designate the current shipping version as the LTS version for the current ecosystem and introduce the radical breaking changes as vNext, effectively rebasing the whole ecosystem.

A potential option 3 is that someone proposes a way of achieving 1 without it being gnarly.

@glopesdev
Copy link

glopesdev commented Nov 9, 2023

@idg10 thank you for your thoughtful review of the situation as always.

I am well aware of these pains. My most important Rx project is stuck in v2.2.5 ever since we got the short end of the stick when Rx changed strong name signature when moving to .NET foundation. All our dependencies structure got permanently broken and no amount of manifest tweaks or redirects can deal with a strong name key change.

However, we have recently found a way to move on that may be applicable here. Essentially we decided to target v2.2.5 for all projects targeting net472 and v4 for projects targeting net5. This was a good solution that allowed us to overlay incompatible dependency graphs in a way that both preserves all our legacy ecosystem and allows people to move forward while being compatible.

I was hoping it would not be too late to do the same for Rx.NET. Essentially the best chance to do a clean break is .NET 6/core. I agree with your assessment that NET FX projects are the ones most stuck with legacy code that cannot be updated. This is where I would draw the line of back compatibility, since .NET core ecosystem is much more dynamic and used to dealing with changes.

So I propose the following: ditch UWP as you propose and keep NET FX UI schedulers on System.Reactive only for net472 targets. For net6 forward, remove the schedulers and ask people to move on and include the new platform-specific packages.

Re. The backcompat facades like Core, etc, I would simply stop updating them to encourage people who can to move on. The goal of these packages should be to resolve back compatibility issues and not for apps actively being maintained.

Microsoft .NET with all its gigantic ecosystem was able to make the decision for a clean break for the sake of supporting sustainable evolution of their ecosystem, my proposal is that we take that opportunity and go along with the ride.

@glopesdev
Copy link

To further generalize the above proposal, if you happen to consider it too late to make .NET 6 the cutoff point, the exact same idea could be applied to the upcoming .NET 8, or future releases. My argument for why this is always appropriate is twofold:

  1. Target platforms represent a contained boundary within the dependency graph, meaning that potentially incompatible yet overlapping dependency trees can coexist within the same package.
  2. Updating a target platform is always something that should be carefully considered for any application, and not to undergo lightly. If something is in legacy maintenance mode for whatever reason, then there is no need to move the target platform and no dependencies will break.

I would still recommend picking an LTS release like .NET 6 or .NET 8 to do the clean break, since those are usually much more likely to be adopted by reusable library maintainers.

@ScarletKuro
Copy link

I believe it would be interesting to receive feedback regarding this change from the opensource developers who are directly working with Rx. If this gets merged, it will affect RxUI, ReactiveProperty etc.
Therefore I'm going to tag @runceel, @ChrisPulman, @glennawatson

@idg10
Copy link
Collaborator Author

idg10 commented Nov 13, 2023

I propose the following: ditch UWP as you propose and keep NET FX UI schedulers on System.Reactive only for net472 targets. For net6 forward, remove the schedulers and ask people to move on and include the new platform-specific packages.

This will break .NET 6.0. .NET 7.0, and .NET 8.0 apps that find themselves in the situation I outlined in my earlier post. I'll copy the relevant part here:

The fundamental problem occurs here if a single application has two dependencies (which might be indirect, non-obvious dependencies) of this form:

  • LibA uses System.Reactive v5.0, and depends on DispatcherScheduler
  • LibB uses System.Reactive v7.0 and uses a V7.0-specific feature (e.g., imagine for the sake of argument that we add a RateLimit operator to deal with the fact that almost nobody likes Throttle)

A brand new .NET 8.0 application written today could find itself in this situation shortly after Rx 7.0 shipped. Since Rx 7.0 doesn't exist yet, any version of LibB that exists today would depend on Rx 6.0 and there's no problem. When building the app, package resolution will unify to Rx 6.0, and both LibA and LibB will be happy, because both will have their needs met by Rx 6.0. But once we release Rx 7.0, if LibB upgrades to Rx 7.0 (and makes use of a new feature in Rx 7.0) and if Rx 7.0 changes System.Reactive in the way you propose, this application (which, don't forget, is a brand new .NET 8.0-based app, not some legacy app) would find itself stuck. The build process will unify to Rx 7, causing LibA to fail when it tries and fails to use DispatcherScheduler. Either the app would be doomed to stick on the older version of LibB forever, or to stop using either LibA or LibB or both, unless it could somehow convince the authors of LibA to release an upgraded version that depends on Rx 7. (But LibA's authors might be reluctant to do that after seeing the problems LibB created for its users when it upgraded to Rx 7.)

The problem here is not confined to .NET FX. In fact it doesn't really affect .NET FX at all. The problem here is very much specific to modern .NET.

I don't yet understand how the "clean break" opportunity you're seeing could work. The basic problem here is that versions get unified: the build system is going to pick exactly one version of System.Reactive. Any future package called System.Reactive can't be a clean break because it may find itself substituted for an older version, and being used by code that was expecting the older version.

If we are truly to have a clean break here, there has to be a change of package identity, with System.Reactive being phased out. (There are "clean break" and "not so clean break" versions of this. In the "clean break" version System.Reactive would basically become set in stone, never changing. In the "no so clean break" version, it becomes a type forwarder to whatever the new package is called. But in either case, you need a new NuGet package.)

@idg10
Copy link
Collaborator Author

idg10 commented Nov 13, 2023

Since I'd like the discussion on this to have higher visibility, I've created this: #2038

@glopesdev
Copy link

@idg10 thanks for creating the discussion, I will continue there, but just to close I would like to add one clarification:

This will break .NET 6.0. .NET 7.0, and .NET 8.0 apps

If we make the cutoff point .NET 8.0 for example, then apps made in .NET 6.0 and .NET 7.0 would not be affected, unless I am fundamentally misunderstanding something about NuGet packaging (which is a genuine possibility).

A brand new .NET 8.0 application written today could find itself in this situation shortly after Rx 7.0 shipped.

This actually to me seems like an example of what we would want. Any "brand new" application written today should not be using legacy packages. If they need to for whatever reason, and those packages aren't upgraded, then they should not target .NET 8.0 and should pick an earlier compatible target.

@ChrisPulman
Copy link
Contributor

Since I'd like the discussion on this to have higher visibility, I've created this: #2038

I have created a response with my thoughts in 2038, thanks Ian.

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.

None yet

5 participants