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

Move UI and platform types out of System.Reactive #2084

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

Conversation

idg10
Copy link
Collaborator

@idg10 idg10 commented Feb 12, 2024

Deprecate all UI-framework-specific and platform-specific types in System.Reactive. Add various System.Reactive.Integration.* packages with the replacement types.

Resolves #1745
Resolves #1847

To be resolved before we can take this out of draft:

  • Community review of ADR and this implementation of it
  • See if anyone can come up with a better name than Integration
  • Add additional ApiApprovalTests members for these new packages
  • Determine whether System.Reactive.Integration.Uwp can be a normal UWP class library so we can have one fewer uses of MSBuild.Sdk.Extras

Deprecate all UI-framework-specific and platform-specific types in System.Reactive. Add various System.Reactive.Integration.... packages with the replacement types.
@idg10 idg10 added this to the Rx 7.0 milestone Feb 12, 2024
@idg10 idg10 self-assigned this Feb 12, 2024
@glopesdev
Copy link

@idg10 this looks great! Is there a reason why we can't drop the "Integration" bit and make these just System.Reactive.WindowsForms, System.Reactive.Wpf, etc? I was searching nuget.org but couldn't find any existing listed package (maybe they are deprecated and unlisted?).

@HowardvanRooijen
Copy link
Collaborator

If you've been keeping track of this and the mammoth ADR that preceded it, the first BIG problem was working out what to do. The second BIG problem is working out what to call it 🤣

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.
@idg10
Copy link
Collaborator Author

idg10 commented Feb 13, 2024

there a reason why we can't drop the "Integration" bit and make these just System.Reactive.WindowsForms, System.Reactive.Wpf

There is already a System.Reactive.Windows.Forms. It exists only as a backwards compatibility facade enabling code that was written against Rx v3 or older to work. It contains just two type forwards, to the existing legacy ControlScheduler and ControlObservable.

I thought that for this thing to end up having two roles:

  • backwards compatibility to Rx v3 and earlier
  • the home for new types that will enable us to deprecate and remove the legacy Windows Forms types

would be confusing, because those are almost diametrically opposed objectives.

Also, the equivalent components for WPF and WinRT are pretty confusingly named in my opinion:

  • System.Reactive.Windows.Threading contains forwarders for either the WPF or the WinRT dispatcher (depending on which TFM you're looking at)
  • System.Reactive.WindowsRuntime contains forwarders for the WinRT support, except for the bits that are in System.Reactive.Windows.Threading

If we were to try to take these existing facade components and turn them back into the home for the new types, would we continue with the existing confusing split? Would we put all the WinRT types in the System.Reactive.WindowsRuntime library this time because there's no real reason for the CoreDispatcher support to be in the WPF library? Would we continue to put WPF support in System.Reactive.Windows.Threading or would we introduce a new library with a name making it more clear that it's a WPF library?

There are a few different workable answers to these questions, but none of them looked very good to me.

The benefits (as I see it) of the new names are:

  • Consistency: they're all called System.Reactive.Integration.*
  • It would be fairly clear how we'd add new integration components if required (e.g., for MAUI)
  • The target UI framework or platform is clear from the name
  • This states clearly that we no longer consider these to be a core part of Rx.NET: they provide integration between Rx.NET and other frameworks or platforms

That said, I don't love the Integration part of the name. I'd be open to other suggestions, but I'd want it to deliver all those same benefits.

@glopesdev
Copy link

@idg10 For some reason I cannot find any of the other compatibility facades except for the WindowsRuntime one. Do you have links for them?

I'm not sure if creating more packages will do anything to alleviate the confusion, in my experience this tends to always increase with number of packages.

I will need to digest this a bit, but "Integration" seems far too abstract.

If absolutely necessary to introduce a new root name I would prefer "PlatformServices" mainly because it has a recognisable meaning dating back to the original Rx architecture.

@glopesdev
Copy link

glopesdev commented Feb 13, 2024

@idg10 I think now I understand what is going on and why I couldn't find any of the existing packages, and it might actually work to our advantage if we are willing to clean everything up properly.

Here is the list of the currently existing "integration" packages:

My proposal would be to name the packages the following:

  • System.Reactive.WindowsForms
  • System.Reactive.Wpf
  • System.Reactive.Uwp
  • System.Reactive.Maui
  • System.Reactive.WindowsRuntime

From this list, my understanding is we have overlap with just one package (System.Reactive.WindowsRuntime). I don't think overlapping the two functionalities of forwarding the backwards compatibility facades and containing the new integration bits are that much of a problem, especially because we will be doing already this hybridization in the core System.Reactive package. If the end goal in several years is to do a full cleanup of the namespaces, I think this is worth it.

The aforementioned naming scheme I think would be the preferred one, since there is an implicit understanding that creating a sub-namespace with a platform or framework name means an integration with that platform or framework (this is used extensively within several existing Microsoft and third-party NuGet package names).

* Add Obsolete attributes
* Add the newly-public (was internal) member of AsyncLock
@idg10
Copy link
Collaborator Author

idg10 commented Feb 13, 2024

From this list, my understanding is we have overlap with just one package (System.Reactive.WindowsRuntime).

True, but we would have both of these:

  • System.Reactive.WindowsForms
  • System.Reactive.Windows.Forms

I don't much like the idea of both of those existing. It's definitely the kind of thing where, if I encountered this in someone else's code I'd wonder what on earth they were thinking.

I don't think overlapping the two functionalities of forwarding the backwards compatibility facades and containing the new integration bits are that much of a problem

Here's one thing to consider. Those libraries still use the funky old versioning trick that #205 introduced in the attempt to work around one a problem that could occur when using Rx.NET in a plug-in. Code built against Rx.NET v3 may still be relying on that versioning trick—it will still have references to each of these older individual components instead of a single reference to System.Reactive making it possible for it to end up with a mismatched set of Rx.NET DLLs.

So those all still have an AssemblyVersion of 3.0.something, even in the 6.0.0 NuGet package.

That's OK today, because we know that any code using these old façade libraries is pretty old, so it presumably hasn't been updated since Rx 3.0. It's OK for the version numbers to have frozen on these things because those libraries haven't actually changed in all that time. But what if we start putting new non-façade code in there? What should the version number be then? If we don't update it, we could run into a problem where applications end up running against older versions than they are meant to. But if we do update it we run into a possible problem with binding redirects. Binding redirects often specify version ranges, which means if you upgrade 3.x to 4.x, it's possible that 3.0.4000.0 would get upgraded to 7.0.4000.0, which could actually mean a downgrade in surface area (because the x.x.4000.0 versions might have target-specific functionality that the x.x.2000.0 versions do not).

I was actually considering marking these façade as deprecated because new code should not be using them, and all of the UI-framework-specific ones currently refer to types we intend to deprecate. It would be an extra tool to nudge people towards using the new types in place of the ones we want to deprecate.

But if we start adding new content, then we break that assumption that code using these libraries is old. There are now two completely different audiences: very old code and very new code.

It's true that I can't see a way in which this is absolutely guaranteed to be broken. (Although I might have missed something.) But it makes me very uneasy.

@JakenVeina
Copy link

JakenVeina commented Feb 22, 2024

System.Reactive.Extensions.Wpf and System.Reactive.Extensions.Uwp?

Me personally, I don't really have any issue with Integration.

@anaisbetts
Copy link
Contributor

anaisbetts commented Feb 22, 2024

Instead of "Integration" just use the word "For"

System.Reactive.For.Uwp
System.Reactive.For.Blazor
System.Reactive.For.WPF

Everyone reading it will know exactly what it does, it's not taken already, and it's way shorter to type than "Integration"

@mikemilleresq
Copy link

Have you considered Targets? very similar to the For suggestion.

@dezfowler
Copy link

I think I'd like to see this follow the recent MS convention of ancillary stuff like frameworks and UI not having the System. prefix.

So have it exist in a separate tree altogether; but what that tree is I don't know as I feel like all the reasonable prefix choices are probably burnt already.

So, for example, you have...

System.Reactive

and then something like...

Reactive.Extensions.Wpf

Or...

ReactiveUI.Extensions.Wpf

@idg10
Copy link
Collaborator Author

idg10 commented Feb 28, 2024

I really like @anaisbetts suggestion of System.Reactive.For... I think it's a big improvement on my Integration.* names.

@dezfowler suggested not using System.Reactive at all. This has its attractions but I see one particular problem with it: it's a lot less clear that packages such as Reactive.Extensions.Wpf are part of the same world as System.Reactive. If we were going to rename everything, including the core Rx.NET package, then perhaps it would make more sense. But we've always wanted to keep System.Reactive as the main package. (And although we initially thought that this wasn't practical, the discovery of a workaround changed that.)

I don't want to completely dismiss the idea (using something other than a System package name) because I think it has its upsides. But I think it is worth highlighting the downsides. If most people like the idea even after any problems have been fully examined, then we shouldn't ignore that.

It's worth noting that because Rx.NET is a .NET Foundation project, we are able to use certain reserved prefixes in our package names. Normally you're not allowed to publish packages with a name starting System, but we get an exemption, and this is part of what enables people to be confident that they are using the right package.

There are already Reactive.* packages out there (e.g. ReactiveUI's Reactive.Wasm so I don't think this is likely to be made a protected package prefix like System and Microsoft are

Supply chain attacks have become very important over recent years, so I really don't want to remove a layer of protection against those that Rx.NET consumers currently get.

So for me, these are the two main arguments against Reactive.Extensions.Wpf etc. 1) It makes it harder to understand that this is the officially supported package provided by the same team that provides System.Reactive. 2) it offers less defence against supply chain attacks.

@dezfowler
Copy link

@idg10 The supply chain risk is a very good point and my suggestion was essentially around risks along those same lines...

It seems the rule Microsoft are working to (which may be imaginary as I haven't actually found reference to this in the Framework Design Guidelines etc) is that System.* as a group of packages including the BCL is self contained and doesn't reference any packages outside of System.*.

So that when a user installs a System.Blah package, they'd like for it to only reference the BCL or other System.* packages with similar provenance, licences, trust levels. Ideally, when I install something System.* I don't want to have to worry about it bringing in a package under GPL, for example.

I notice that in amongst the packages on Nuget tagged with rxteam is this one...

image

https://www.nuget.org/packages/Microsoft.Reactive.Testing/6.0.0

In this comment it seems this Microsoft.Reactive prefix was used to satisfy some "nuget guidelines" which may be the imaginary rule I'm looking for but I can't find that doc either.

If this Microsoft.Reactive prefix is already reserved for this project then maybe we could consider that as an option for any packages that reference ancillary things?

@glopesdev
Copy link

glopesdev commented Feb 28, 2024

I really like @anaisbetts suggestion of System.Reactive.For... I think it's a big improvement on my Integration.* names.

@idg10 I feel this suggestion is not acceptable for the following reasons:

  • For is a reserved keyword in VB and only differs in casing from the reserved keyword for in C#. If we introduce System.Reactive.For namespace, this means that simply using System.Reactive can make this identifier visible all over existing and future codebases.
  • In English, the word "for" as used here is a preposition linking two nouns which is only meaningful when read in its entirety. However, namespaces in C# can be pulled apart leaving the preposition meaningless. Again taking the common example of using System.Reactive this means we can then write an expression such as For.WinForms.ControlScheduler.Schedule which to me becomes extremely ambiguous in terms of meaning to the naive reader.

Finally, I don't think I have ever seen packages or namespaces named using a composition of dot-separated verbs and nouns meant to be read as a sentence, and most definitely not in the System namespace. If I was a consumer of such a package I think it would make me triple-check whether this is really the official package.

True, but we would have both of these:

  • System.Reactive.WindowsForms
  • System.Reactive.Windows.Forms

I agree these two forms should not coexist, and to me the solution here is simply to deprecate all legacy integration packages. This way it would make it clear to users what is the way forward.

I understand the argument for backwards compatibility, but what we are discussing here goes beyond that, and is starting to create undue burden on the project maintainers and the rest of the community. Who is really using these packages and how do we assess their impact? Download counts is not enough unfortunately because these could be automated CI workflows or other deployment artifacts so I doubt they will give us a clear idea of the real impact of the change.

These packages were designed in the .NET framework era which has been left behind by Microsoft itself. The .NET core team had no qualms to remove core features of .NET such as application domains, CompileToMethod and so many other great features in the name of cleaning up the ecosystem. Why do we, as an open-source community with drastically more limited resources, need to hold ourselves to a stricter standard and bend over backwards in the name of packages which only target UAP or .NET framework?

I keep having the strong feeling we are coming up with proposals that risk compromising the future of a great library by introducing further and further confusion, and I think continuing to update and support these old facade packages just keeps sending the wrong message to users, especially if they are advertised using names which are the most natural to look for.

If we do have to give up on the natural names, then to me the best proposal I have seen so far is to go with System.Reactive.Extensions.*, which at least stays true to the original spirit of Rx being "Reactive Extensions to the .NET framework", where now we can have not just one but multiple "frameworks".

@HowardvanRooijen
Copy link
Collaborator

I really like @anaisbetts suggestion of System.Reactive.For... I think it's a big improvement on my Integration.* names.

@idg10 I feel this suggestion is not acceptable for the following reasons:

  • For is a reserved keyword in VB and only differs in casing from the reserved keyword for in C#. If we introduce System.Reactive.For namespace, this means that simply using System.Reactive can make this identifier visible all over existing and future codebases.
  • In English, the word "for" as used here is a preposition linking two nouns which is only meaningful when read in its entirety. However, namespaces in C# can be pulled apart leaving the preposition meaningless. Again taking the common example of using System.Reactive this means we can then write an expression such as For.WinForms.ControlScheduler.Schedule which to me becomes extremely ambiguous in terms of meaning to the naive reader.

Finally, I don't think I have ever seen packages or namespaces named using a composition of dot-separated verbs and nouns meant to be read as a sentence, and most definitely not in the System namespace. If I was a consumer of such a package I think it would make me triple-check whether this is really the official package.

True, but we would have both of these:

  • System.Reactive.WindowsForms
  • System.Reactive.Windows.Forms

I agree these two forms should not coexist, and to me the solution here is simply to deprecate all legacy integration packages. This way it would make it clear to users what is the way forward.

I understand the argument for backwards compatibility, but what we are discussing here goes beyond that, and is starting to create undue burden on the project maintainers and the rest of the community. Who is really using these packages and how do we assess their impact? Download counts is not enough unfortunately because these could be automated CI workflows or other deployment artifacts so I doubt they will give us a clear idea of the real impact of the change.

These packages were designed in the .NET framework era which has been left behind by Microsoft itself. The .NET core team had no qualms to remove core features of .NET such as application domains, CompileToMethod and so many other great features in the name of cleaning up the ecosystem. Why do we, as an open-source community with drastically more limited resources, need to hold ourselves to a stricter standard and bend over backwards in the name of packages which only target UAP or .NET framework?

I keep having the strong feeling we are coming up with proposals that risk compromising the future of a great library by introducing further and further confusion, and I think continuing to update and support these old facade packages just keeps sending the wrong message to users, especially if they are advertised using names which are the most natural to look for.

If we do have to give up on the natural names, then to me the best proposal I have seen so far is to go with System.Reactive.Extensions.*, which at least stays true to the original spirit of Rx being "Reactive Extensions to the .NET framework", where now we can have not just one but multiple "frameworks".

I believe the idea is to use the for convention just for the package name, not for the internal namespaces (for many of the reasons you highlighted).

@glopesdev
Copy link

glopesdev commented Mar 5, 2024

I believe the idea is to use the for convention just for the package name, not for the internal namespaces (for many of the reasons you highlighted).

@HowardvanRooijen thanks for the clarification, I agree the packages should be functional in that case. However, System.Reactive.For.Uwp still sounds uncanny to me. I guess the reason is that even if For is not an actual namespace name, it still makes up part of the assembly qualified name, with an enforced dot-separated identifier which conveys at least some notion similar to a namespace identifier.

Also the point remains that the biggest source of confusion is that the "legacy" packages are still being maintained. They should be actively deprecated, and their updates discontinued as soon as possible. If we keep updating them and System.Reactive.Windows.Forms still shows up on NuGet side-by-side with System.Reactive.For.WindowsForms, I somehow don't think it will be very clear to people which one they should pick up, and even less clear if they use the command-line.

I don't have many other arguments and apparently it doesn't bother anybody else, so I'll leave it at that, but for the record this doesn't look good to me.

@dezfowler
Copy link

That same comment I linked above says that the package name should match...

the dominant namespace in each package

So if the intent is to not have a .For. namespace then we'd be breaking that convention.

As I said, though, I would really like to see all of these integration packages using Microsoft.Reactive instead of System.Reactive... especially where they're targeting a Microsoft technology which uses Microsoft.* package naming itself.

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