josepot
commented
4 years ago Is this all correct?
Partially, yes.
Is this "safe" for effectful streams?
Most definitely. We have been using these bindings at my company for some months now: we use lots of streams that trigger side-effects (websocket connections, http requests, etc) and we have never ran into the kind of issue that you are describing.
If subscribing to a stream triggers side effects like a network request, this could be really important to users for it to be documented
As I already mentioned in a previous issue: we are still working on the docs site, and this in particular will be properly explained.
However, I think you are missing some context:
1) What you are describing would only happen on a component that's being mounted and that is leveraging the Suspense API because that stream has never emitted anything yet. Also that's assuming that the user is not using the useSubscribe hook above the Suspense component and that the user hasn't increased the graceTime of the hook.
2) I could argue that if you have a component makes React take longer than 200ms to commit on a mounting component after the suspense promise resolves, then it's very possible that you actually have a bigger issue already :sweat_smile:
3) However, even if that was the case, the user would have at least 2 ways to avoid that hypothetical issue:
- Use the second argument of
bindto increase thegraceTimeto let's say a couple of secs. - Use
useSubscribejust above theSuspensecomponent.
For instance, let's say that that following code is problematic:
const [useUser] = bind(id => requestUser(id));
const User: React.FC<{userId: number}> = ({ userId }) => {
const user = useUser(userId);
return (
<ComponentVeryVeryExpensiveToMount user={user} />
);
}
export const SuspendedUser: React.FC<{userId: number}> = ({ userId }) => (
<Suspense fallback={null}>
<User userId={userId} />
</Suspense>
)The problem could easily be solved like this:
const [useUser, getUser$] = bind(id => requestUser(id));
const User: React.FC<{userId: number}> = ({ userId }) => {
const user = useUser(userId);
return (
<VeryExpensiveToMountComponent user={user} />
);
}
export const SuspendedUser: React.FC<{userId: number}> = ({ userId }) => {
useSubscribe(getUser$(userId))
return (
<Suspense fallback={null}>
<User userId={userId} />
</Suspense>
);
}or like this:
const [useUser] = bind(id => requestUser(id), 5_000);
const User: React.FC<{userId: number}> = ({ userId }) => {
const user = useUser(userId);
return (
<VeryExpensiveToMountComponent user={user} />
);
}
export const SuspendedUser: React.FC<{userId: number}> = ({ userId }) => (
<Suspense fallback={null}>
<User userId={userId} />
</Suspense>
)Also, you may find this passing test interesting. FTR: currently, that test would probably not pass on the experimental version of React. However, it does pass on the latest release of React 17, so I think that we are on a good spot right now :slightly_smiling_face:.
All that being said, I think that I got an idea to further mitigate this (IMO very hypothetical) issue.
joshribakoff
voliva
In the same vein as my concerns in https://github.com/kosich/react-rxjs-elements/issues/4 I walked through your library to see how it works, it is very clever!
However, I see that in render phase, the library is calling
toPromise(essentially subscribing in render). Would it be accurate to say that the way this library works is as following:connectObservablecausing their stream to gain a monkey patchedgetValue()methodinitthunk touseReducerinitthunk is invoked by react, in the render phase, which callsgetValue()getValuecauses the stream to become subscribed (it callstoPromise())useEffecthook runs, and subscribes a second time (to the shared observable, so the source stream is only subscribed once)useEffecthook will run causing a second subscription (to a new instance of a shared observable, so the source stream is subscribed twice as well)Is this all correct? If subscribing to a stream triggers side effects like a network request, this could be really important to users for it to be documented as a tradeoff the library makes, so they can know this upfront instead of learning it the "hard way" after writing an app.
If I understand, there is a tradeoff here where: