Skip to content

React / Sutil inspired state handling#179

Merged
sleepyfran merged 42 commits intomasterfrom
universal-hooks
Jan 29, 2022
Merged

React / Sutil inspired state handling#179
sleepyfran merged 42 commits intomasterfrom
universal-hooks

Conversation

@JaggerJo
Copy link
Copy Markdown
Member

@JaggerJo JaggerJo commented Dec 12, 2021

This is a prototype for a React/Sutil inspired state handling library for FuncUI.

There is an example contact app included to test it.

Screenshot 2021-12-13 at 00 23 49

Feedback is appreciated.

Comment on lines +5 to +16
type IConnectable =
inherit IDisposable
abstract InstanceId: Guid with get

type ITap<'signal> =
inherit IConnectable
abstract member CurrentSignal: 'signal with get
abstract member Subscribe : ('signal -> unit) -> IDisposable

type IWire<'signal> =
inherit ITap<'signal>
abstract member Send : 'signal -> unit
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not really happy with the naming here. Open for suggestions.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if there would be value in utilizing the built-in IObservable<'T>, IObserver<'T> and Subject<'T> for the underlying pub/sub mechanism.
While I admittedly don't quite have my head wrapped around the nuances of everything Wire.fs, it seems like it is very similar.

Potential benefits might be:

  • Mainly, the ability for an end user to take advantage of more advanced Rx capabilities on the incoming subscriptions.
    For example: if you wanted to share an IObservable<string> of keystrokes from one component to another, the user could use Rx debounce to limit observed messages to avoid unnecessary refreshes. This is a fairly common request that would be a pain to implement manually.
  • A secondary benefit would be a familiar pub/sub model with known names (IObservable instead of ITap, Subject instead of IWire, etc).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Initially considered this, but IObservable does not contain a value.

I had to look it up, but IEvent already inherits from IObservable, so it seems that the door is still open to allowing integration with Rx.

open Avalonia.Layout
open Avalonia.FuncUI

let contactListView (contacts: ITap<Contact list>, selectedId: IWire<Guid option>, filter: IWire<string option>) =
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

example usage

@JordanMarr
Copy link
Copy Markdown
Collaborator

This looks really cool!

@JordanMarr
Copy link
Copy Markdown
Collaborator

Looking at your ContactBook example now...
Your componentized app is very clean. I love it! So happy to see this.

@JaggerJo
Copy link
Copy Markdown
Member Author

JaggerJo commented Dec 14, 2021

Looking at your ContactBook example now...
Your componentized app is very clean. I love it! So happy to see this.

Great to hear!

There are a few things I'm not yet sure about:

  1. Naming. IWire is terrible, not sure if IReadable + IWritable or a variation of it would be better.

  2. When using a useState hook in react its value is consistent during one render.

let counter = React.useState 0
React.useEffectOnce (fun _ -> 
    counter.set (counter.current + 1)
    counter.set (counter.current + 1)
    counter.set (counter.current + 1)
)
counter.set (counter.current + 1)
counter.set (counter.current + 1)
counter.set (counter.current + 1)

// current render 'counter' = 0
// next render 'counter' = 1

Not sure yet if we should copy this behaviour.

  1. We currently depend on call order or hooks like react. I don't really like this as we can't conditionally add hooks to the component.

Alternatives to make a call to ctx.useWhatever unique could be:

@JordanMarr what do you think?

@JaggerJo JaggerJo changed the title [WIP] React / Sutil inspired state handling [ WIP | Discussion ] React / Sutil inspired state handling Dec 14, 2021
@JordanMarr
Copy link
Copy Markdown
Collaborator

  1. Naming. IWire is terrible, not sure if IReadable + IWritable or a variation of it would be better.

I like publish/subscribe because it is used a lot in UI frameworks for sharing between view models in MVVM.

Rx uses Observable/Observer/Subject (where Subject can publish and subscribe). Still not as clear as publish/subscriber though IMO.

  1. When using a useState hook in react its value is consistent during one render.
let counter = React.useState 0
React.useEffectOnce (fun _ -> 
    counter.set (counter.current + 1)
    counter.set (counter.current + 1)
    counter.set (counter.current + 1)
)
counter.set (counter.current + 1)
counter.set (counter.current + 1)
counter.set (counter.current + 1)

// current render 'counter' = 0
// next render 'counter' = 1

Not sure yet if we should copy this behaviour.

I don’t have a strong opinion because I am wary of multiple renders in React and so I would never do this - I would just set one state at time. I also only use a single useState at a time to prevent confusing multi pass render scenarios.

  1. We currently depend on call order or hooks like react. I don't really like this as we can't conditionally add hooks to the component.

Alternatives to make a call to ctx.useWhatever unique could be:

I am already used to React not allowing conditional hooks, so this limitation doesn’t really bother me.

@Zaid-Ajaj
Copy link
Copy Markdown

One of the most exciting features I have seen in a while for F# on desktop 😍 looking forward to dig into this further.

One thing I didn't see in the example is the array dependencies for effects which re-triggers the effect when one of it's dependencies change:

// currently this is the API usage
let contact = ctx.useTap contact
let image = ctx.useAsync Api.randomImage

// In case the image is _dependant_ on the contract
let contact = ctx.useTap contact
let image = ctx.useAsync(Api.contactImage contact.Id, [ contact.Id ])

Copy link
Copy Markdown
Collaborator

@JordanMarr JordanMarr left a comment

Choose a reason for hiding this comment

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

The new names are much more representative of their intended functionality in this context. Very nice!

)

let mainView () =
Component (fun ctx ->
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When should one use the Component ctor vs Component.create?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The constructor returns a Component (inherits ContentControl) while the create function returns a View<Component> that can be used in the DSL context.

So one if for the DSL context and one if for Avalonia context.

Comment on lines +5 to +16
type IConnectable =
inherit IDisposable
abstract InstanceId: Guid with get

type ITap<'signal> =
inherit IConnectable
abstract member CurrentSignal: 'signal with get
abstract member Subscribe : ('signal -> unit) -> IDisposable

type IWire<'signal> =
inherit ITap<'signal>
abstract member Send : 'signal -> unit
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if there would be value in utilizing the built-in IObservable<'T>, IObserver<'T> and Subject<'T> for the underlying pub/sub mechanism.
While I admittedly don't quite have my head wrapped around the nuances of everything Wire.fs, it seems like it is very similar.

Potential benefits might be:

  • Mainly, the ability for an end user to take advantage of more advanced Rx capabilities on the incoming subscriptions.
    For example: if you wanted to share an IObservable<string> of keystrokes from one component to another, the user could use Rx debounce to limit observed messages to avoid unnecessary refreshes. This is a fairly common request that would be a pain to implement manually.
  • A secondary benefit would be a familiar pub/sub model with known names (IObservable instead of ITap, Subject instead of IWire, etc).

@JaggerJo
Copy link
Copy Markdown
Member Author

JaggerJo commented Dec 16, 2021

@JordanMarr

I wonder if there would be value in utilizing the built-in IObservable<'T>, IObserver<'T> and Subject<'T> for the underlying pub/sub mechanism.
While I admittedly don't quite have my head wrapped around the nuances of everything Wire.fs, it seems like it is very similar.

Potential benefits might be:

Mainly, the ability for an end user to take advantage of more advanced Rx capabilities on the incoming subscriptions.
For example: if you wanted to share an IObservable of keystrokes from one component to another, the user could use Rx debounce to limit observed messages to avoid unnecessary refreshes. This is a fairly common request that would be a pain to implement manually.
A secondary benefit would be a familiar pub/sub model with known names (IObservable instead of ITap, Subject instead of IWire, etc).

Initially considered this, but IObservable does not contain a value.

IValue could/should implement IObservable I think, but It's not sufficient on it's own.

https://fsprojects.github.io/FSharp.Data.Adaptive/ could work instead of our own values, but I don't fully get it. From looking at the code it does a lot I don't understand (yet).

@JaggerJo
Copy link
Copy Markdown
Member Author

One of the most exciting features I have seen in a while for F# on desktop 😍 looking forward to dig into this further.

One thing I didn't see in the example is the array dependencies for effects which re-triggers the effect when one of it's dependencies change:

// currently this is the API usage
let contact = ctx.useTap contact
let image = ctx.useAsync Api.randomImage

// In case the image is _dependant_ on the contract
let contact = ctx.useTap contact
let image = ctx.useAsync(Api.contactImage contact.Id, [ contact.Id ])

@Zaid-Ajaj Thanks for the kind words. I have time next week to hopefully make some progress on this. If you have any suggestions just let me know. Happy to discuss anything.

@sleepyfran
Copy link
Copy Markdown
Member

I haven't had a chance to properly check all of the code, but I gotta say that this looks absolutely amazing and really promising! 😃

Just a couple of thoughts I have in my head after a quick check:

  • After your latest commit I have to say that the naming is much clearer now, but I feel like the hooks could be more self-explanatory like: useValue -> useState; usePassedValue -> usePassedState or useExternalState. Basically to better define the notion that a value can be modified and acts as a state holder. Wouldn't say it's a must since the current naming is not bad at all, but it took me a second to realize that useValue meant creating a state.
  • I was going over the list of built-in React hooks and I think making our own version of useEffect (I'd say even two: useEffect that takes a dependency array and a useOnStartEffect or useEffectOnce that executes only the first time) and useMemo would be a great addition to the current bunch.
  • useAsync is an awesome idea and makes such a common thing incredibly easy to implement, awesome job!

@JaggerJo
Copy link
Copy Markdown
Member Author

JaggerJo commented Dec 18, 2021

I haven't had a chance to properly check all of the code, but I gotta say that this looks absolutely amazing and really promising! 😃

Still an early prototype. Effects are tricky, me might need to change a lot to support them properly. Wet paint everywhere 😁

Just a couple of thoughts I have in my head after a quick check:

  • After your latest commit I have to say that the naming is much clearer now, but I feel like the hooks could be more self-explanatory like: useValue -> useState; usePassedValue -> usePassedState or useExternalState. Basically to better define the notion that a value can be modified and acts as a state holder. Wouldn't say it's a must since the current naming is not bad at all, but it took me a second to realize that useValue meant creating a state.

Like your suggestions, would favor names that are different from react. I fear (and I might be wrong) that having similar names with different behavior could confuse people.

Think we should just use the names you suggested for now tho. Might be good to also rename 'Value' to 'State' then 🤔

  • I was going over the list of built-in React hooks and I think making our own version of useEffect (I'd say even two: useEffect that takes a dependency array and a useOnStartEffect or useEffectOnce that executes only the first time) and useMemo would be a great addition to the current bunch.

I have an implementation that adds effects locally. It currently is super buggy, will post here in a few days with more details/ the problems that come with effects.

We should not run effects during render or directly when a dependency changes I think.

So we need to have something like an 'Executor' that either runs effects from some kind of queue or renders the component. But never both at the same time.

  • useAsync is an awesome idea and makes such a common thing incredibly easy to implement, awesome job!

The 'useAsync' hook should be based on a state + effect hook I think - so that we can support reloading/invalidation.

@JaggerJo
Copy link
Copy Markdown
Member Author

JaggerJo commented Dec 20, 2021

Update

Better Names

Component (fun ctx ->
    let contacts = ctx.usePassedState ContactStore.shared.Contacts
    let selectedId = ctx.useState (None: Guid option)
    let filter = ctx.useState (None: string option)
    ..
)

Conditionally render hooks.

This is archived by using the line number a hook was invoked from as the identity instead of the call order. The line number is implicitly passed.

type Context with
    member this.useState<'value>(init: 'value, [<CallerLineNumber>] ?identity: int) : IWritable<'value> =
        this.useHook<'value>(
            StateHook.Create (
                identity = $"Line: {identity.Value}",
                state = StateHookValue.Lazy (fun _ -> new State<'value>(init) :> IConnectable),
                renderOnChange = true
            )
        ) :?> IWritable<'value>

..

Effects

Render only once after component is initialised / after first render

Component (fun ctx ->
    this.useEffect (
        handler = (fun _ ->
            ..
        ),
        triggers = [ EffectTrigger.AfterInit ]
    )
    ..
)

Render after every render.

Component (fun ctx ->
    this.useEffect (
        handler = (fun _ ->
            ..
        ),
        triggers = [ EffectTrigger.AfterRender ]
    )
    ..
)

Render when dependencies changed.

Component (fun ctx ->
    let contacts = ctx.usePassedState ContactStore.shared.Contacts
    let selectedId = ctx.useState (None: Guid option)

    this.useEffect (
        handler = (fun _ ->
            ..
        ),
        triggers = [ 
            EffectTrigger.AfterChange selectedId.Any
            EffectTrigger.AfterChange selectedContact.Any
        ]
    )
    ..
)

@Zaid-Ajaj @JordanMarr @sleepyfran Thoughts?

@JordanMarr
Copy link
Copy Markdown
Collaborator

@Zaid-Ajaj @JordanMarr @sleepyfran Thoughts?

My thoughts are that you are on 🔥!

I like the React-y hook names.

One question about re-rendering when dependencies change:

        triggers = [ 
            EffectTrigger.AfterChange selectedId.Any
            EffectTrigger.AfterChange selectedContact.Any
        ]

Is it possible to pass in any value on the model to AfterChange?
For example passing an arbitrary property that lives on the useState model -- or does it always check the entire model?

@JaggerJo
Copy link
Copy Markdown
Member Author

@Zaid-Ajaj @JordanMarr @sleepyfran Thoughts?

My thoughts are that you are on 🔥!

I like the React-y hook names.

One question about re-rendering when dependencies change:

        triggers = [ 
            EffectTrigger.AfterChange selectedId.Any
            EffectTrigger.AfterChange selectedContact.Any
        ]

Is it possible to pass in any value on the model to AfterChange?

EffectTrigger.AfterChange works with anything that implements the IAnyReadable interface.

Types that implement Readable<'t> (like State<'t>) are extended to have an Any : IAnyReadable property.

type IAnyReadable =
abstract member SubscribeAny : (obj -> unit) -> IDisposable
type internal AnyReadable<'value>(readable: IReadable<'value>) =
interface IAnyReadable with
member this.SubscribeAny (handler: obj -> unit) : IDisposable =
readable.Subscribe handler
[<AutoOpen>]
module IReadableExtensions =
type IReadable<'value> with
member this.Any with get () : IAnyReadable =
AnyReadable<'value>(this) :> IAnyReadable

For example passing an arbitrary property that lives on the useState model -- or does it always check the entire model?

Right now it does not even check if there was really a change to the value. It just subscribes to the passed dependencies. Those dependencies might have a unique value filter.

It's also possible to "focus" state and add a unique value filter to it.

let readUnique (state: IReadable<'value>) : IReadable<'value> =
let uniqueWire = new UniqueValueReadOnly<'value>(state)
uniqueWire :> _
let readMap (mapFunc: 'a -> 'b) (value: IReadable<'a>) : IReadable<'b> =
new ValueMapped<'a, 'b>(value, mapFunc) :> _

Component.create ("Person", fun ctx ->
    let name =
        person
        |> State.readMap (fun person -> person.FullName)
        |> State.readUnique
        |> ctx.usePassedState (* the context already supports state that does not trigger a render on change. there are just no overloads for it yet.  *) 

    this.useEffect (
        handler = (fun _ ->
            ..
        ),
        triggers = [ 
            EffectTrigger.AfterChange name.Any
        ]
    )

    ..
)

@JaggerJo
Copy link
Copy Markdown
Member Author

JaggerJo commented Dec 22, 2021

To test more advanced scenarios I ported an app (FSharpGeneticAlgorithm) from @IntegerMan to FuncUI and the new state management.

There are still a few small bugs I need to fix, but it went pretty smooth overall.

Screenshot 2021-12-22 at 20 02 24

@Zaid-Ajaj
Copy link
Copy Markdown

@JaggerJo This looks absolutely amazing 😍 😍 can't wait to try it out!

@JordanMarr
Copy link
Copy Markdown
Collaborator

Nice!
I plan on trying out your component system by recreating my chord parser UI in the very near future.
Of course I will need to contrive a reason to have more than one screen to fully test the components aspect. 🤔

@JaggerJo
Copy link
Copy Markdown
Member Author

Nice!
I plan on trying out your component system by recreating my chord parser UI in the very near future.
Of course I will need to contrive a reason to have more than one screen to fully test the components aspect. 🤔

Could be a nice app to include in the Examples if that would be ok for you.

@JaggerJo
Copy link
Copy Markdown
Member Author

It would be nice if useState behaved the same way as it does in React and Feliz by returning a tuple.

Ex:

let filter, setFilter = useState filter

This is nice because it makes your UI code easier to write by eliminating the extra calls to .Set and .Current.

However, I realize that IWritable<'value> is fundamental for passing stuff between components.

Maybe there could be a different useState overload that works like classic React / Feliz?

Alternatively, maybe there could be a simple built-in function to easily convert an IWritable<'value> to a tuple:

/// Needs a better name that this...
let split (writable: IWritable<_>) = 
        writable.Current, writable.Set

Usage

let enabled, setEnabled = ctx.useState true |> split

I personally never liked the way react does this and would not include a function that seperates current and set right now.

It can easily be added tho by people not liking the current model - but if you ever need to pass that state down you need to reconstruct a Readable somehow, making the events work, ... so I would not encourage doing this by including it in the lib.

@JaggerJo
Copy link
Copy Markdown
Member Author

Super busy right now unfortunately.

This is ready to merge after combining the FuncUI and FuncUI.Components projects. Would someone take over that part?

@sleepyfran
Copy link
Copy Markdown
Member

Super busy right now unfortunately.

This is ready to merge after combining the FuncUI and FuncUI.Components projects. Would someone take over that part?

Awesome to hear it's ready! I'll take care of combining both modules, solving all the conflicts and merging tomorrow 😃

@sleepyfran sleepyfran changed the title [ WIP | Discussion ] React / Sutil inspired state handling React / Sutil inspired state handling Jan 18, 2022
@JaggerJo
Copy link
Copy Markdown
Member Author

JaggerJo commented Jan 19, 2022

Super busy right now unfortunately.
This is ready to merge after combining the FuncUI and FuncUI.Components projects. Would someone take over that part?

Awesome to hear it's ready! I'll take care of combining both modules, solving all the conflicts and merging tomorrow 😃

IIRC there is already a Component namespace + folder in the FuncUI project - but it's meaning is a bit different. Might be good to change that as I see the could lead to confusion.

@JaggerJo
Copy link
Copy Markdown
Member Author

@sleepyfran let me know if you need help!

@JaggerJo JaggerJo mentioned this pull request Jan 26, 2022
@sleepyfran
Copy link
Copy Markdown
Member

@sleepyfran let me know if you need help!

Thanks :)

I actually haven't had that much time, but I started the process the other day on the branch universal-hooks-moving, having some compilation issues that for some reason didn't arise when the code was in a separate project, but as I said I haven't had much time to look into it, hopefully I can finish the work soon.

How should we name the code that is currently sitting in the components folder, though? It's just the Hosts, Lazy and DataTemplateView, so not sure what the correct naming for that one should be.

@JaggerJo
Copy link
Copy Markdown
Member Author

@sleepyfran let me know if you need help!

Thanks :)

I actually haven't had that much time, but I started the process the other day on the branch universal-hooks-moving, having some compilation issues that for some reason didn't arise when the code was in a separate project, but as I said I haven't had much time to look into it, hopefully I can finish the work soon.

How should we name the code that is currently sitting in the components folder, though? It's just the Hosts, Lazy and DataTemplateView, so not sure what the correct naming for that one should be.

No worries, can relate. Time is flying by right now.

Regarding the stuff currently in the components folder:

  • Move hosts & data template views to top-level namespace
  • Delete Lazy (components should be used instead)

@sleepyfran
Copy link
Copy Markdown
Member

@sleepyfran let me know if you need help!

Thanks :)
I actually haven't had that much time, but I started the process the other day on the branch universal-hooks-moving, having some compilation issues that for some reason didn't arise when the code was in a separate project, but as I said I haven't had much time to look into it, hopefully I can finish the work soon.
How should we name the code that is currently sitting in the components folder, though? It's just the Hosts, Lazy and DataTemplateView, so not sure what the correct naming for that one should be.

No worries, can relate. Time is flying by right now.

Regarding the stuff currently in the components folder:

* Move hosts & data template views to top-level namespace

* Delete Lazy (components should be used instead)

Thanks a lot! I finished the whole thing and published the PR in #188, take a look whenever you get some time, but this should be ready to roll! 😃

# Conflicts:
#	src/Avalonia.FuncUI/Avalonia.FuncUI.fsproj
#	src/Examples/Examples.CounterApp/Examples.CounterApp.fsproj
@sleepyfran
Copy link
Copy Markdown
Member

It looks like there were still some conflicts even after merging my branch, should be good now. Also bumped the dotnet version in the Actions so that they don't fail while trying to compile the Chord Parser project 😃

@sleepyfran sleepyfran merged commit 1b397dd into master Jan 29, 2022
@sleepyfran sleepyfran deleted the universal-hooks branch January 29, 2022 17:52
@sleepyfran
Copy link
Copy Markdown
Member

We're live 🚀

Thanks @JaggerJo for the amazing work 😃

@JaggerJo
Copy link
Copy Markdown
Member Author

@sleepyfran nice! @JordanMarr

How do we go about releasing this? I guess we should also change the package name to not include 'Jaggerjo'.

1 similar comment
@JaggerJo
Copy link
Copy Markdown
Member Author

@sleepyfran nice! @JordanMarr

How do we go about releasing this? I guess we should also change the package name to not include 'Jaggerjo'.

@sleepyfran
Copy link
Copy Markdown
Member

@sleepyfran nice! @JordanMarr

How do we go about releasing this? I guess we should also change the package name to not include 'Jaggerjo'.

Agree, we were having that conversation as part of #173. My favorite option was FSharp.Avalonia as @JordanMarr suggested, but I'm up for whatever other naming scheme. I've never published anything on NuGet, is it enough with renaming the Product/PackageId fields in the fsproj file or is there any other change required in NuGet itself?

@sleepyfran sleepyfran mentioned this pull request Jan 29, 2022
4 tasks
@sleepyfran
Copy link
Copy Markdown
Member

Also created #189 to track the steps needed for before/after the release of the new version so that the docs and other resources are in good health, feel free to suggest any other steps to be added 😃

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.

4 participants