feat(bind): factory observable functions should receive a key

[josepot]

This is a breaking change, but I really think that this is the way to go. I think that the factory overload of bind should only take one argument: the key, and it doesn't have to be a primitive.

I don't want to rush this, but I'm 99.99% sure that this is the way to go. @voliva what do you think?

[voliva]

I think overall it's nice that it accepts any kind of key now, not only literals, so I think that this would be a positive change. But it feels like we're compromising with having only one parameter.

What's stopping us from accepting any number of params? I believe that technically it's achievable, but at the same time I can't find an example where it would be essential.

Even if we think an example that has a pair of values that identify something (e.g. repo URL + page), the consumer can make another stream which has the latest of both in an object, and that will hold the same reference until one of those values change. The problem I see now, is that if the value changes back to a previous one, then the reference would be a new one again, and not the one of the past. Is there a way around this? Does it matter? Or it's a case where accepting more than one key is essential?

Sorry I've typed this while I was thinking about it - Anyway, it's nice to understand this train of thought.

[codecov]

Codecov Report

Merging #77 into main will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #77   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          293       291    -2     
  Branches        35        35           
=========================================
- Hits           293       291    -2     

Impacted Files	Coverage Δ
packages/core/src/bind/connectFactoryObservable.ts	100.00% <100.00%> (ø)
packages/core/src/bind/index.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 031faad...be85cd1. Read the comment docs.

[josepot]

What's stopping us from accepting any number of params? I believe that technically it's achievable

The thing is that those params have to end up becoming a key, I think that it's better not to try to hide that from the users.

Even if we think an example that has a pair of values that identify something (e.g. repo URL + page), the consumer can make another stream which has the latest of both in an object, and that will hold the same reference until one of those values change.

Or the consumer can have functions for serializing/deserializing those keys, e.g:

const getRepoKey = (repo: string, page: number): string => JSON.stringify([repo, page])
const parseRepoKey = (key: string): [string, number] => JSON.parse(key)

and then use those.

Also, it's worth noting that with the overload that I'm proposing in this PR it would be possible to create a custom bind that behaves like the current overload:

const parse = <A>(key: string): A => JSON.parse(key)  
const getKey = <A>(args: A): string => JSON.stringify(args) 
                                                                               
export const bindFactory = <A extends (number | string | boolean | null)[], O>(
  getObservable: (...args: A) => Observable<O>,  
  unsubscribeGraceTime: number,
) => 
  bind( 
    (key: string) => getObservable(...parse<A>(key)),
    unsubscribeGraceTime
  ).map((fn) => (...args: A) => fn(getKey(args))) as [
    (...args: A) => Exclude<O, typeof SUSPENSE>,
    (...args: A) => Observable<O>,
  ]

Therefore, I really think that with these changes we would be exposing a better primitive.

At the end of the day there has to be a key that identifies each instance of the factory, trying to "hide" that from the consumer doesn't create a better abstraction IMO, on the contrary.

[josepot]

A point could be made that this doesn't have to be a breaking change and that we could just add another overload. If we weren't on major version zero I think that would be the way to go, but since we are still on major version zero I think that this is the right approach. I want to sleep on it, because maybe it's worth it to keep the current overload... 🤔

[voliva]

Working with serializers kills ergonomy though (been there, done that :P)

My idea is that we could use a multi-key data structure (e.g. nested Maps instead of just Map<key, observable>) to actually support multiple keys, just like a database would. So it's not like we'd be abstracting over using one single key and hiding this fact to the consumer, but just supporting multiple keys by itself.

[josepot]

Working with serializers kills ergonomy though (been there, done that :P)

That's true.

My idea is that we could use a multi-key data structure (e.g. nested Maps instead of just Map<key, observable>) to actually support multiple keys, just like a database would.

That's an interesting idea! #78 implements that and the CI passes, but I would like to add more tests. I will do that as soon as I have some time. Another nice thing about this approach is that on top of being more ergonomic, it doesn't imply breaking changes. @voliva feel free to help me improve #78 if you have the time and you feel like it...

The only drawbacks are: increase in bundle size... I initially thought that perhaps the perf would get worse, but I don't think so, because we no longer have to serialize/deserialize the arguments, so perf-wise it should be about the same, right? 🤔

We should have some benchmarks to measure these things...

[josepot]

Closing this in favour of #78