raveclassic / frp-ts

Functional reactive values-over-time
MIT License
80 stars 8 forks source link

[Question] Duped values - do not propagate events or simply return cached value #71

Open Nedgeva opened 1 year ago

Nedgeva commented 1 year ago

Hi there! I have question regarding proper approach on working with properties which has duped value. How to obtain them when atoms have deduping logic on set? Well pretty easy - just put some complex data structure into atom (e.g. object) and then derive property with primitive value. So what confusing me is that previously (in 0.0.x versions) we had some operators like map: https://github.com/raveclassic/frp-ts/blob/8c97587eda37dbf30cef11d3432bebd5163b7b23/src/property.ts#L59-L65 where incorporated memoizing would do what it's intended to do - skipping re-evaluation of provided fn on same arg, not stopping propagating value down the chain.

Now I see a bit different technique involved in combine operator (it's essentially the same as map): https://github.com/raveclassic/frp-ts/blob/6c2dac9df9477aa12af156d2612f4691ca0cef54/packages/core/src/property.ts#L104-L110

so now combine would just skip propagating event so observers will not be notified in case of duped value.

Earlier I was thinking that deduplication/filtering is in full responsibility of value producer and should happen right before setting it to atom (except for builtin triple eq checking in atom.set), but now I'm not sure that this approach was right and it's looks like it's totally ok to pull-down circuit breaker somewhere along the way of evaluation chain.

@raveclassic @Fyzu can you share your thoughts please?

raveclassic commented 1 year ago

Hey @Nedgeva!

To be honest, this implicit "filtering" behavior bugs me a bit as it doesn't bring any real value but only brings pure performance overhead. Notification propagation is internal to the library and, since value read is pure pull, implicit memoization in combine (and others) will return the same value. On top of that, useProperty will skip duplicates as well and won't trigger setState.

Indeed, previously we've been notifying consumers in some cases when the value in producer technically was the same. Now, combine filters out such notifications.

I'm not 100% sure this is the right approach, but this is how it's implemented now. Please share your thoughts on this as well.

Nedgeva commented 1 year ago

@raveclassic thanks for explanation! I found it's interesting topic for discussion. And I do understand why you implement things as it is.

My thoughts are:

Have you thought of sort of RFC for frp-ts?