-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Update to build with Avalonia 11 Preview 5 #246
Conversation
@@ -33,7 +33,7 @@ module internal Extensions = | |||
member this.SubscribeWeakly(callback: 'a -> unit, target) = | |||
Observable.subscribeWeakly(this, callback, target) | |||
|
|||
type IInteractive with | |||
type Interactive with |
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.
Another change that effects this bit is that the Avalonia change at AvaloniaUI/Avalonia#9749 has removed its usage of System.Reactive and replaced it with a bunch of (mostly internal) things.
I added a local reference to System.Reactive to make it build, but i'm not sure of the 'best' approach (still learing how all this hangs together) - e.g. Avalonia now seems to have an internal implementation of Observable.Create that uses a minimal local class, so I wondered if doing something like
{ new IObservable<'args>
with member this.Subscribe(observer: IObserver<'args>) = sub.Invoke(observer) }
would work instead of importing Reactive just for Observable.Create?
There are also 3 references to Observable.Subscribe that require Reactive, but the comment at AvaloniaUI/Avalonia#9749 (comment) suggests there might be more changes coming on the Avalonia side there
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.
should be good 👍
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.
The general bit about System.Reactive, or the local object expression suggestion?
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'd prefer not having a reference to 'System.Reactive'. So the object expression seems good to me.
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.
Would there be any value in breaking bits like this into separate PRs (in cases where they can be done without updating the Avalonia version)?
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.
The two cases where breaking this PR into smaller bits might make sense:
- if the change could be released as a new preview version that would provide immediate value for end users.
- if the change is big enough (touches enough files) that it will become harder to merge the longer it sits.
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.
The question was mainly that you don't have to do it all in one go (as you could leave the FuncUI code using a local System.Reactive reference to begin with and fix it later, or you could try removing local dependencies on System.Reactive without updating Avalonia, e.g. #261 ).
The value is probably more that it'd be more self contained to test.
There is also a break in the web assembly playground project due to Avalonia.Web.Blazor having been renamed to Avalonia.Browser.Blazor, but that doesn't doesn't seem to currently be attached to the solution? |
No, The WASM Example even had runtime errors last time I tried it with v11. |
395c347
to
72759ee
Compare
Ok, I've updated the project anyway, something else to test with the new versions |
3ed5ec6
to
9db7e66
Compare
Another breaking change has appeared in the Avalonia 11 nightlys - the change at AvaloniaUI/Avalonia#9677 has changed some virtualization related things, which causes a number if build breaks due to properties having been removed or renamed |
597baa6
to
f181f7d
Compare
f33e0d9
to
24104c6
Compare
I was holding off on trying to make a couple of other System.Reactive uses ( type IObservable<'a> with
member this.Subscribe (callback: 'a -> unit, token: CancellationToken) =
let disposable = Observable.subscribe callback this
token.Register(fun () -> disposable.Dispose()) might work? |
Updated with a couple more breaking API changes in Avalonia (this time just in the sample apps though) |
Removed the ItemsRepeater bindings after AvaloniaUI/Avalonia#10112 |
a5c13bd
to
1a4e129
Compare
Ok, updated to Avalonia 11 preview 5. Seems to be working ok, but still has a System.Reactive reference to get a couple of uses of |
Wonder when all of this great stuff will land in a new public avalonia preview 😀 |
Nice! Looking forward to merging your changes and also releasing a preview version. |
…g on System.Reactive
hmm, forgot about the web assembly playground project, might need more tweaks there (although, it could maybe also be simplified to use the new self-hosted WASM support rather than needing Blazor?) |
Whatever works 😉 |
I've updated the playground project so it builds with Preview 5, but I got a javascript error when running it - so that bit needs another look later |
I doesn't work currently, so we can definitely fix it after merging. 👍 |
Merged! Great work 🔥 |
A WIP change, to try to make it build against the latest Avalonia 11 nightly build.
Most of the changes are to replace uses of interfaces with their base classes to make it work with the Avalonia changes from AvaloniaUI/Avalonia#9553
Note: I've had the control catalog working with thest changes, except that horizontally resizing the window corrupts some of the contents. That may or may not be down to the issue described at AvaloniaUI/Avalonia#9975