connectFactoryObservable: Object parameters will cause infinite loops

[SebKranz]

Hey, thanks for the project. You saved me a lot of headaches.

The following code will lead to an infinite loop when used in a component:

export type ArticleQuery = {
	searchTerm?: string,
	author?: string
}

const [useArticleQuery] = bind((query: ArticleQuery) => searchForArticles$(query))

This is likely, because the NestedMap only expects an array of arguments and doesn't deeply compare objects.

I would suggest taking a look at the system that react-query uses for key generation. They sort the entries of an object before converting it to json. (See the function stableStringify)

Maybe it is also somehow possible to detect when a loop occurs and throw an error instead of potentially sending many requests to a server.

[josepot]

👋 @SebKranz

Thanks a lot for your kind words. I'm very sorry that you found yourself in that situation. The "Factory Overload" is very tricky. We should improve the docs to explain it better. We should add examples that illustrate the different ways that it can be leveraged, and we should also explain why we have decided to compromise its ergonomics in order to avoid memory leaks.

I would suggest taking a look at the system that react-query uses for key generation. They sort the entries of an object before converting it to json. (See the function stableStringify)

This library is very different from react-query. IMO it's a lot more powerful and perf-sensitive than react-query because it allows consumers to have full-control over their caches. Also, accessing a stream stored on a factory bind is a constant time operation (O(1)), and we must keep it like that because the function is evaluated on every react update.

The bindings on @react-rxjs/core will never do the kind of "magic" that react-query does, because doing that would be detrimental for many of the use-cases for which this library was created.

However, we are aware that there are many use-cases for which the core API is not ergonomic. That's why have created the @react-rxjs/utils package. We will keep improving it to accommodate other common patterns.

Maybe it is also somehow possible to detect when a loop occurs and throw an error instead of potentially sending many requests to a server.

This is a really good suggestion. I think that it would be possible to do some of those checks on the development build and to show warnings when we detect a case that it's likely to fall for this pitfall. Thanks for the suggestion! We will act on it, for sure! Also, we will improve our eslint plugin to detect some of these cases.

It will take us a few days to update the docs and I would like to help you solve your particular problem before then. However, I'm afraid that I don't have enough context to help you. It's possible that you don't even need to use the "Factory Overload". Could you please give us more context on the problem that you are trying to solve? A codesandbox with a basic proof of concept would be ideal.

Thanks and looking forward to hearing back from you.

[SebKranz]

Hey, apollogies for the late replay.

My use case is that I have some url-parameters that would be pointless to store as global state since they're handled by my router.

I use these parameters to make a request to a server. (With a cache in between thats not unlike react-query)

Because the endpoint has many optional parameters, and because these parameters are passed through multiple functions, I collect them in an object and create a type for them.

But more generally, I think that objects are an elegant way of describing complex API's since JS only supports positional arguments.

On a side note: I believe that it's important to be able to easily switch between the reactive and imperative style. There are cases where the imperative approach is more intuitive or even required (eg. by a framework).

connectFactoryObservable is an important bridge to all kinds of other frameworks (router, useState, or even useSelector). Therefore, maybe it should be more flexible, even at the cost of performance?
However, will first read the updated documentation, so that I can better understand the reasons behind your decision.

[josepot]

Hi @SebKranz ,

I'm very sorry for the late response.

On a side note: I believe that it's important to be able to easily switch between the reactive and imperative style. There are cases where the imperative approach is more intuitive or even required (eg. by a framework).

💯

connectFactoryObservable is an important bridge to all kinds of other frameworks (router, useState, or even useSelector). Therefore, maybe it should be more flexible, even at the cost of performance?

Yes, you are correct connectFactoryObservable is an important bridge for covering the impedance mismatch between the 2 different mental models. Although, I would rather leave it the way it currently is and add a util that provides the extra-functionality that you are looking for.

However, will first read the updated documentation, so that I can better understand the reasons behind your decision.

Yesterday we did a small update. It's still far from perfect, but it does explain the most important things. The next thing that I'm going to do is to add an example in the tutorial that explains in detail other use-cases and gotchas of the parametric bind. I'm very sorry that it's taking us so long to address this.

My use case is that I have some url-parameters that would be pointless to store as global state since they're handled by my router.

I use these parameters to make a request to a server. (With a cache in between thats not unlike react-query)

Because the endpoint has many optional parameters, and because these parameters are passed through multiple functions, I collect them in an object and create a type for them.

But more generally, I think that objects are an elegant way of describing complex API's since JS only supports positional arguments.

In a situation like the one that you are describing, I think that the best way to approach that problem (taking into account the "philosophy" of this library) would be to have a stream that listens to the url changes, then you can use that stream to create the query and the article, something more or less like this:

const [useArticle] = bind(url$.pipe(
  map(getQueryFromUrl),
  switchMap(searchForArticles),
))

If for some reason, you also needed to access the ArticleQuery from somewhere else, then you could do this:

const [useArticleQuery, articleQuery$] = bind(url$.pipe(
  map(getQueryFromUrl),
))

const [useArticle] = bind(articleQuery$.pipe(
  switchMap(searchForArticles)
))

Please let me know what your thoughts are or if you have suggestions for better (or more ergonomic) ways of approaching these problems. Although, please be mindful of the motivation of the library and the trade-offs that we have chosen. The trade-offs, we can change them, of course, as long as they don't compromise the main goals of the library.

Sorry again for getting back to you so late. I will try to be a lot more responsive in the future.

[SebKranz]

Hey, so I guess I won the contest for the most delayed reply in this thread ;)
I have no excuse other than: I forgot.

Your suggested solution works and seems reasonable.

When I realize that the amount of state-information that needs to be passed through the react-hierarchy is usually small, the limitation appears far less restrictive.

So far, I haven't come across other use-cases where I needed to pass a large amount of arguments to bind.