-
Notifications
You must be signed in to change notification settings - Fork 750
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
Add decision record for packaging changes #2086
base: main
Are you sure you want to change the base?
Conversation
…aving anything to do with WPF
After some experiments, we can rule out certain type forwarder based approaches
Got as far the great unification section
Updated as far as "The workaround" section
Rx.NET/Documentation/adr/0003-windows-tfms-and-desktop-framework.md
Outdated
Show resolved
Hide resolved
Rx.NET/Documentation/adr/0003-windows-tfms-and-desktop-framework.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Steven Weerdenburg <stevenaw@users.noreply.github.com>
Co-authored-by: Steven Weerdenburg <stevenaw@users.noreply.github.com>
We will go with [option 6](#option-6-ui-framework-specific-packages-deprecating-the-systemreactive-versions): | ||
|
||
* `System.Reactive` will remain as the main package for using Rx.NET | ||
* we will add a new NuGet packages for each UI framework, and put UI-framework-specific support in these |
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.
* we will add a new NuGet packages for each UI framework, and put UI-framework-specific support in these | |
* we will add new NuGet packages for each UI framework, and put UI-framework-specific support in these |
Minor grammar issue. I agree it is fair to say the majority of users has agreed with this proposal.
|
||
* `System.Reactive` will remain as the main package for using Rx.NET | ||
* we will add a new NuGet packages for each UI framework, and put UI-framework-specific support in these | ||
* there will be an additional package for functionality specific to Windows Runtime that is not UI-framework-specific |
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.
* there will be an additional package for functionality specific to Windows Runtime that is not UI-framework-specific | |
* there will be an additional package for functionality specific to Windows Runtime that is not UI-framework-specific (e.g. integration with `IAsyncOperation<T>`) |
I was confused about this aspect without additional context, so thought it would be worth clarifying with an example for those who are only reading this section.
|
||
> it didn't make much sense in a layer map to have those dependencies for something as generic as an event processing library. As a result, we refactored out the UI dependencies in System.Reactive.Windows.Forms.dll (for Windows Forms) and System.Reactive.Windows.Threading.dll (for WPF and Silverlight). | ||
|
||
The benefit of hindsight makes the wisdom of this evident. There were some good reasons for the _great unification_ but many of the factors that made it look like the best option at the time no longer apply, and changes in the .NET world that occurred after the _great unification_ have turned it into a problematic decision. |
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.
I still find it shocking why this was not flagged earlier on, and why everything about Rx was allowed to undergo breaking changes, including strong name key pair changes which completely prevent assemblies from loading (with or without facades).
|
||
Although there are some existing old packages that used to contain some UI-framework-specific types, they are now backwards-compatibility facades. However, these don't separate out all the frameworks completely: Windows Forms has its own one, `System.Reactive.Windows.Forms`, but the WPF support lives in a more confusingly named `System.Reactive.Windows.Threading`, and that same component also defines the UWP integration. The WPF support is available only in the `net472` build, and the UWP support is only in the `uap10.0.18362` build, so this one package contains completely different types in its two target frameworks! And just to confuse matters, there's also a `System.Reactive.WindowsRuntime` package that targets only `uap10.0.18362`, and which contains some different UWP types. | ||
|
||
Our current plan is to leave these facade components untouched, since the split of types is a bit confusing, the naming is a bit inconsistent, and their purpose for years has been to provide backwards compatibility for Rx v3 era apps. We will introduce new components each with a clear purpose: |
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.
If we don't deprecate, archive and / or unlist the existing facade packages, I fear that the confusion will persist. I don't feel creating a new System.Reactive.For.WindowsForms
package will have sufficient legitimacy to displace the older (and better named) packages.
* `System.Reactive.For.Uwp` | ||
* `System.Reactive.For.WindowsRuntime` | ||
|
||
We expect people to hate these names on first acquaintance. (Our initial proposal was to have `Integration` instead of `For`, and people definitely hated that. Anais Betts suggested this `For` name instead. We liked it more, as did most other people, but all options are at least slightly controversial.) All other suggestions to date have either had problems (e.g. there are already packages with the suggested names) or aren't noticeably better. |
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.
We liked it more, as did most other people, but all options are at least slightly controversial.) All other suggestions to date have either had problems (e.g. there are already packages with the suggested names) or aren't noticeably better.
There were a number of alternative proposals discussed in #2084 and I don't remember seeing a significant enough expression of appreciation for any of them to consider the matter decided.
I have updated that thread with support for the suggestion from @dezfowler which now stands at 3 votes to 6 on the suggestion from Anais Betts. For the record, I do think his suggestion is noticeably better than the proposal using For
.
One of the main goals of Rx 7.0 is to address the packaging problems in which applications with Windows-specific TFMs may find their deployment sizes ballooning by up to 90MB.
The issues go back a long way and are surprisingly complex. It will take several releases to fully fix this while allowing existing Rx.NET users enough time to make changes where required.
This ADR describes the history, the design options considered, the proposed way forward, and the expected consequences.