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.

Sign up for GitHub

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

Jump to bottom

Remove System.Reactive package #9749

Merged
merged 12 commits into from
Jan 9, 2023
Merged

Remove System.Reactive package #9749

merged 12 commits into from
Jan 9, 2023

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Dec 21, 2022 •
edited
Loading

What does the pull request do?

Replaces System.Reactive package with internal helpers.

Why?

Problems with System.Reactive package:

  1. It forces users to include WPF and WinForms packages on net6.0-windows builds, see System.Reactive reference pulls in all of WPF/SWF when TFM is net7.0-windows10.0.19041.0 or higher #9549
  2. It bloats package size especially with AOT/R2R scenarios, as this dependency is full of generics NETSDK1082 if installed on WinUI project targeting 19041 or higher dotnet/reactive#1461 (comment)
  3. Similarly to Linq it's just better to avoid abstractions like this, but in the PR I tried to replace with the same code.

Breaking changes

  1. Any usage of ISubject in the public API was replace with IObservable or IObserver or pair of both, depending on the context.
  2. Unit type usage was replaced with object type.
  3. AvaloniaScheduler was moved to Avalonia.ReactiveUI package.
  4. If developers rely on System.Reactive package, but didn't use ReactiveUI, they might need to include System.Reactive manually, as it won't be a transitive dependency anymore from avalonia.

Fixed issues

Fixes #9549

@maxkatz6 maxkatz6 requested a review from a team December 21, 2022 02:53
maxkatz6
maxkatz6 commented Dec 21, 2022
View reviewed changes
src/Avalonia.Base/Reactive/LightweightObservableBase.cs
public IDisposable Subscribe(IObserver<T> observer)
{
_ = observer ?? throw new ArgumentNullException(nameof(observer));

Dispatcher.UIThread.VerifyAccess();
//Dispatcher.UIThread.VerifyAccess();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grokys @kekekeks I have no idea, but for some reason VerifyAccess really slows down application here.

LightweightSubject implementation inherits this LightweightObservableBase, and pointer events using it feel really and really slow. Even though Subscribe call isn't actually executed on pointer movements.

Commenting this line out magically removes the problem.
I also checked, there are no exceptions thrown here, and replacing method with bool CheckAccess() call doesn't make it better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc this is a hot path so adding a thread access verification might really slow things down... but that's just my conjecture though.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0027802-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@hez2010
Copy link
Contributor

hez2010 commented Dec 22, 2022

Tested this PR and saw a nearly 20% dramatically size improvement on NativeAOT-ed CC.NetCore. Really impressive.

@Mikolaytis
Copy link
Contributor

Wow! Finally, this is happening!

@maxkatz6
Copy link
Member Author

@hez2010 it should be even better together with another PR #9760. Unless you already have referenced specific packages instead of whole Avalonia.Desktop.

@maxkatz6
Merge remote-tracking branch 'origin/master' into remove-reactive-pac…
a92bf0a 
…kage

# Conflicts:
#	src/Avalonia.Base/Animation/AnimationInstance`1.cs
#	src/Avalonia.Base/Data/InstancedBinding.cs
@maxkatz6 maxkatz6 force-pushed the remove-reactive-package branch from 837623c to a92bf0a Compare December 22, 2022 22:11
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0027846-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0027883-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@ScarletKuro ScarletKuro mentioned this pull request Dec 25, 2022
Open
@maxkatz6
Merge remote-tracking branch 'origin/master' into remove-reactive-pac…
4ca0dba 
…kage

# Conflicts:
#	src/Avalonia.Base/Input/Gestures.cs
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028427-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@hez2010
Copy link
Contributor

hez2010 commented Jan 8, 2023 •
edited
Loading

I'm wondering if we can also remove the System.Reactive dependency from MiniMvvm and ControlCatalog?

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028437-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028439-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

jmacato
jmacato approved these changes Jan 9, 2023
View reviewed changes
Copy link
Member

@jmacato jmacato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jmacato jmacato enabled auto-merge January 9, 2023 10:31
@jmacato jmacato merged commit d2bfd61 into master Jan 9, 2023
@jmacato jmacato deleted the remove-reactive-package branch January 9, 2023 10:57
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0028447-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@TomEdwardsEnscape
Copy link
Contributor

Why are the replacement helper classes internal? This PR breaks features as simple as AvaloniaProperty.Changed.Subscribe. @maxkatz6, please make Avalonia.Reactive.Observable and its dependencies public!

We could of course add the Reactive package to our application, but a) that defeats the purpose of this PR and b) new Avalonia developers aren't going to know which package to add.

For now I'll take a copy of the "internal" helper classes and put them in my own solution.

@maxkatz6
Copy link
Member Author

maxkatz6 commented Jan 9, 2023 •
edited
Loading

@tomenscape this PR is more about having a choice - to include or not System.Reactive.
In general, we don't want to maintain public reactive extensions by ourselves, even if it's couple of API methods like Subscribe, Select, Where...
There are also would be conflicts if both namespaces (system.reactive and avalonia.reactive) are included and public.

@TomEdwardsEnscape
Copy link
Contributor

I can see your concerns about the very low-level methods and classes. But please reconsider at least for the Observable.Subscribe and Observable.Select methods. These are used to listen for properties changing and to provide inline value conversion when binding in C#. Both are common activities that are no longer possible with vanilla Avalonia.

AvaloniaObjectExtensions.AddClassHandler is still public, so I don't see why these other two shouldn't be as well. Move them into the extension call and give them semantic names if you want, but please spare new developers the headache of trying to work out how to implement all this stuff.

@maxkatz6
Copy link
Member Author

AvaloniaObjectExtensions.AddClassHandler is still public

AFAIK, it doesn't conflict with reactive extensions.

But please reconsider at least for the Observable.Subscribe and Observable.Select methods

I see why Subscribe is needed for the developers, especially not as experienced, who don't know about reactive extensions much. That's something that we should revisit now...

But I don't see any reason to add public extensions for the Select method. Without full set of reactive extension (which we will not provide), this method isn't as useful. And with combination with Subscribe method you can simply do mapping in the subscribe handler.

@TomEdwardsEnscape
Copy link
Contributor

TomEdwardsEnscape commented Jan 11, 2023 •
edited
Loading

Select statements can be sent straight back into Avalonia. Here's an example of how we use them to provide value conversion:

textBlock.Bind(Layoutable.WidthProperty, border.GetObservable(Visual.BoundsProperty).Select(r => r.Width));

The GetObservable and Select calls could be rolled up into a method with a signature similar to GetValueConversionObservable<TSource,TResult>(this AvaloniaObject, AvaloniaProperty<TSource>, Func<TSource,TResult>).

@Numpsy Numpsy mentioned this pull request Jan 15, 2023
Merged
@amwx amwx mentioned this pull request Jan 24, 2023
Closed
@maxkatz6 maxkatz6 mentioned this pull request Nov 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jp2masa jp2masa jp2masa left review comments

@kekekeks kekekeks kekekeks left review comments

@kronic kronic kronic left review comments

@jmacato jmacato jmacato approved these changes

Assignees
No one assigned
Labels
breaking-change feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

System.Reactive reference pulls in all of WPF/SWF when TFM is net7.0-windows10.0.19041.0 or higher
9 participants
@maxkatz6 @avaloniaui-team @hez2010 @Mikolaytis @TomEdwardsEnscape @kronic @kekekeks @jp2masa @jmacato