re-rxjs / react-rxjs

React bindings for RxJS
https://react-rxjs.org
MIT License
549 stars 19 forks source link

Is @react-rxjs/core@0.6.0-1 a significant breaking change from 0.5.0? #153

Closed ilikejames closed 3 years ago

ilikejames commented 3 years ago

Upgrading to @react-rxjs/core@0.6.0-1 I can't get the most basic PoC to work.

import React from "react";
import { bind, shareLatest } from "@react-rxjs/core";
import { timer } from "rxjs";
import { startWith } from 'rxjs/operators';

const [useTimer] = bind(() => timer(0, 1000).pipe(startWith(0), shareLatest()));

export default function App() {
  const timer = useTimer();
  return <div className="App">{timer}</div>;
}
Error
Missing subscription

Code

If this is due to breaking changes, a suggestion would be to expand this exception message to give more detail. Plus, to add a demo package with a few working examples to provide end users with a sanity check.

If it is a regression, perhaps there can be some integration tests added.

josepot commented 3 years ago

Hi @ilikejames,

Yep, there was an important breaking change on 0.6 and we haven't had time to update the docs, yet. I'm very sorry about that. However, the breaking change was documented in the release-notes of 0.6.0. I like the suggestion of improving the exception message. I will leave this issue open until we've addressed that.

We also have to update the docs to show different ways of handling subscriptions. However, in the meanwhile, please have a look at the way that we are handling the subscriptions in the re-architected version of ReactiveTrader. More concretely, I think that these 2 examples can be pretty helpful: the analytics component, and the live-rates component.

Also, something worth pointing out is that since v0.6.0, bind accepts a second optional argument for the default-value. If this argument is provided, then the hook will never trigger Suspense. The reason being that the hook will return the provided default-value whenever the stream hasn't emitted or if the stream emits SUSPENSE, one thing to notice about this, though, is that the initial-subscription will happen after the component gets mounted, not on render. Therefore, if you already know what the intial-value is going to be, and you don't care that the subscription takes place after mounting the component, then you could use that overload and not have to worry about having to deal with subscriptions.

ilikejames commented 3 years ago

Thanks for the comments, but I’m afraid I’m not following.

Both examples you gave use the <Subscribe/> component. Whereas, LineChart.tsx appears to bind a stream directly, with no magic I can see following all the upstream.

From my understanding of:

Breaking change: We are no longer making the initial subscription on render. It's now the responsibility of the user to make sure that the initial subscription is there before the hook consumes the **observable.

That this could be achieved with something like:

const _timer$ = timer(0, 1000)
_timer$.subscribe()
const [useTimer, timer$] = bind(() => _timer$.pipe(startWith(0), shareLatest()));

export default function App() {
  const timer = useTimer();
  return <div>{timer}</div>;
}
 Missing Subscription

Or...

const [useTimer, timer$] = bind(() => timer(0, 1000).pipe(startWith(0), shareLatest()));

export default function App() {
  useSubscribe(timer$);
  const timer = useTimer();
  return <div>{timer}</div>;
}
 Missing Subscription

... or both should work, but don't.

We are using a lot of pre-0.6.0 code so I really like not to have to wrap everything with this component... Is there a similar 5-ish line example that you can point me to? I feel like I've had to parse a whole lot of ReactiveTrader and still no wiser.

josepot commented 3 years ago

Thanks for the comments, but I’m afraid I’m not following.

Sorry, I guess that I should have provided an explanation with more details.

Both examples you gave use the component.

Yes, but notice that what is being passed into the source$ of Subscribe is a stream that merges the streams of its children components.

Whereas, LineChart.tsx appears to bind a stream directly, with no magic I can see following all the upstream.

That's not accurate, the LineChart component can only be used if the subscription to the linechart$ stream that it exposes happens before the component gets rendered. Please, notice how the same file not only exports the LineChart component, it also exports a lineChart$ stream, which is the "subscription" stream that this component needs in order to work. It's the responsibility of its consumer to ensure that the subscription happens before render, which can be accomplished in many different ways.

If we have a look at its consumer: the ProfitsAndLoss component, then we can see how that component is using 2 different components that need streams to work: LastPosition and LineChart.

Now, this component could use Subscribe and not propagate a stream to its parent. However, that's not the behavior that we want: we want to "suspend" the whole analytics section while one of its 3 main components is still loading its data, therefore ProfitAndLoss also exports a profitAndLoss$ stream that its consumer will have to subscribe to before rendering the ProfitAndLoss component, and so on so forth... until there is a moment that we want to create a Suspense boundary, in that moment we use Subscribe (which is a super-set of Suspense) and that ensures that the subscription to that stream will take place before the children components are rendered. Think of Subscribe like a "provider".

We think that this pattern is very powerful, because thanks to it we can compose the "subscription" streams, in the same way that we compose components, and decide at which level we want to stop propagating the streams.

useSubscribe is pretty much useless since 0.6 and we are planning on removing it from the API.

We are using a lot of pre-0.6.0 code so I really like not to have to wrap everything with this component...

You don't have to, you have options: you can use the pattern that I'm describing above, you can use the default-value of bind, you can use top-level subscriptions whenever needed, or a "router" that handles the subscriptions. We will try to document all these options as soon as we can. There have been other projects that had a lot of code in 0.5 and they have successfully migrated to 0.6. I admit that it can be a bit tedious, and I sincerely apologize for that. However, the good news is that 0.5 is forward compatible with 0.6 and that can make the migration a bit less of a pain. That's why I will keep patching 0.5.

Once again, I'm very sorry that you had to go through this.

ilikejames commented 3 years ago

No need to apologise, though perhaps in future 0.6.0 could have been released as a prerelease version before the docs were updated ;)

I follow your last part and see that...

const [useTimer, timer$] = bind(
  timer(0, 1000).pipe(shareLatest())
, 0);

...works.

And after discussion with @voliva, using a default value in this way bypasses Suspense completely. So this is an option for the majority of our streams, and then we only need to use <Subscribe/> on the few stream that should trigger suspenseful behaviour in the ui.

Ok. Thanks!

voliva commented 3 years ago

Hey @ilikejames - glad that you're finding a solution.

A quick note however, just to clarify:

const [useTimer, timer$] = bind(
  timer(0, 1000).pipe(shareLatest())
, 0);

const [useTimerSum, timerSum$] = bind(
  timer$.pipe(scan((a,b) => a+b))
);

In this example, useTimer can be used without needing a <Subscribe /> boundary, but useTimerSum does, even if theoretically it wouldn't dispatch suspense, so it will throw "Missing subscription" if used straight away.

This is because bind can't really know if a stream is doing something that would become asynchronous. To avoid this (without using <Subscribe />), useTimerSum also would need a defaultValue.

Edit: Also, the shareLatest() in this example is redundant, you shouldn't need it.

hoclun-rigsep commented 2 years ago

Do I understand correctly then that <Subscribe> once again requires the source$ prop?

voliva commented 2 years ago

Do I understand correctly then that <Subscribe> once again requires the source$ prop?

No, that hasn't changed. From core@0.8.0 (released late April) it became optional, and this hasn't changed since then.