Copy link

Collaborator

JordanMarr commented 6 days ago

This looks really cool!

JordanMarr commented 5 days ago

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

JaggerJo commented 5 days ago •

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:

• implicitly passing a key to ctx.useWhatever that we supply via the CallerLineNumberAttribute
• explicitly passing a key

@JordanMarr what do you think?

JordanMarr commented 5 days ago

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:

• implicitly passing a key to ctx.useWhatever that we supply via the CallerLineNumberAttribute
• explicitly passing a key

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

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 ])

JaggerJo commented 3 days ago •

@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 commented 3 days ago

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 commented yesterday

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 commented 19 hours ago •

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.