Open darkyzhou opened 1 year ago
Any comments? Would love to open a PR for this.
Hi thanks for the info and sorry for the late reply. I have not come up with a clean and safe solution for calling complete
inside components. If you have an idea on how to fix it PR is welcomed.
Two approaches I could think of:
useObservable
would change, so that we can use useEffect
to complete the old BehaviorSubject
and replace with a new one. But this approach requires listing deps explicitly like other hooks useXX(() => {}, [...deps])
which is cumbersome and a breaking change.complete
with setTimeout
in useEffect
so that extra effects triggered in StrictMode
are cancelled. This is hacky but should work as expected.Thank you for the replies.
As for the first approach, I think it introduces too many breaking changes and breaks the simplicity of the library.
As for the second one, I don't get what you mean. By "extra effects", do you mean the fact that in StrctMode the cleanup function of useEffect
will be called twice? I think at least we could introduce another ref like firstEffectRef
to work around that.
Not just the cleanup but the useEffect
will run twice. https://react.dev/reference/react/StrictMode#fixing-bugs-found-by-re-running-effects-in-development
Sorry for the late reply. I get what you are referring to and notice the difficulties of leveraging the useEffect()
to call complete()
. While I understand the need to defer to the next macrotask or microtask, it's not always feasible due to the risk of leaking setState()
calls(e.g. users of useObservable()
might accidentally call setState()
after the component is destroyed).
An alternative could be to require users to disable StrictMode
, similar to what LeetCode does, as StrictMode
does not seem to be a useful utility. I personally find it counterproductive that useEffect()
is called twice simply to prevent "user mistakes", especially when it compromises the ability to synchronize with external systems like RxJS. What's more frustrating is the apparent lack of a way to detect whether it's in StrictMode
.
With the release of React 18, it seems that useEffect()
no longer truly functions as an effect, but rather as a "side-effect-less effect". The expectation that the cleanup function should completely reverse the effects of the hook seems unrealistic. This reminds me of a time when I attempted to use it as an onMount()
hook to display a toast to the user upon the creation of a component, only to be surprised by two toasts appearing. Both the document and stackoverflow say I am misusing the hook while not being able to offer me a "right" way to achieve the goal.
The changes to useEffect()
in StrictMode
, in my opinion, have not been for the better. It feels like the essence of what made it so useful has been lost, and it's become more of a hindrance than a help in certain scenarios.
The reason that React calls useEffect
twice in StrictMode is that by design, React wants you to create side effect and clean it up in a single useEffect
, so that in concurrent mode, the React scheduler would have the ability to interrupt a rendering and resume at some point later.
I too think this is an unrealistic design and go against ergonomics. They tried to push it for over two years and responses from the community does not seem well (too hard or even impossible for library authors to implement).
Nevertheless, I still want to keep it from breaking in StrictMode. The LeetCode team has moved away from rxjs-hooks
for quite a while that is why I created observable-hooks
.
I also lean toward the second approach. To avoid delaying complete
maybe we can detect the StrictMode ahead and skip the first cleanup directly.
I'm of the opinion that this doesn't relate to the concurrent mode features. Everything that takes place within useEffect()
occurs during the commit phase. In concurrent mode, React does indeed interrupt, but this only happens during the render phases. Once we reach the commit phase, the process becomes uninterruptible. The only rationale I can conceive for the React team pushing this forward, despite reservations and skepticism from the community, is that it might stem from a strategic decision made by a high-ranking member of the team.
Detecting StrictMode
seems challenging, as <StrictMode>
is essentially a Symbol like Fragment
, and the real magic unfolds within the reconciler. I'm keen on exploring potential solutions that could align with StrictMode
. As for now, I'm considering forking the project and tailoring it exclusively for React 17 and earlier versions since the project I'm working on still uses React 16.
Just stumbling across this issue. It's not clear to me that there is an actual problem here @darkyzhou, but maybe I'm not fully understanding it.
The issue at hand is that the internal BehaviorSubject
created by useObservable
is, depending on how it's used, retaining a reference to a shareReply
observer created by the initialization factory function provided to useObservable
. This observer should be removed when useObservable
is unmounted, but that's not happening. Is that correct?
If this is accurate, I don't think this actually matters as, in this scenario, the BehaviorSubject
is also unmounted and free to be garbage collected. The observer will still be garbage collected at the same time the BehaviorSubject
is. It looks like the two will always be garbage collected when the use of shareReplay
is, so it doesn't look like there's an actual problem here.
Am I misunderstanding something @darkyzhou ? Is it possible for the observable created by shareReply
to be otherwise freed from memory before the BehaviorSubject
is?
Just stumbling across this issue. It's not clear to me that there is an actual problem here @darkyzhou, but maybe I'm not fully understanding it.
The issue at hand is that the internal
BehaviorSubject
created byuseObservable
is, depending on how it's used, retaining a reference to ashareReply
observer created by the initialization factory function provided touseObservable
. This observer should be removed whenuseObservable
is unmounted, but that's not happening. Is that correct?If this is accurate, I don't think this actually matters as, in this scenario, the
BehaviorSubject
is also unmounted and free to be garbage collected. The observer will still be garbage collected at the same time theBehaviorSubject
is. It looks like the two will always be garbage collected when the use ofshareReplay
is, so it doesn't look like there's an actual problem here.Am I misunderstanding something @darkyzhou ? Is it possible for the observable created by
shareReply
to be otherwise freed from memory before theBehaviorSubject
is?
You are right. As for garbage collection there seems to be no problems. Sorry for not being clear about what I am actually referring to for "resource leaks".
When I mention "resources", I'm referring to ongoing side effects that need to be cancelled or explicitly signaled to stop when the Observable
s they are replying on is complete()
-ed. When these Observable
s, created by the hook, are consumed locally within a component, it appears to be fine as the subscriptions are cancelled when the component is destroyed. However, potential issues could arise when these Observable
s are referenced across multiple components or modules that do not share the same lifecycle.
However, potential issues could arise when these
Observable
s are referenced across multiple components or modules that do not share the same lifecycle.
Is this possible though? The only failure scenario that I can see occurs when shareReplay
is used inside the factory function of useObservable()
. And in this case, the observable created by the factory function will always be local to that component (and never reused elsewhere). If you pass in an already made observable (i.e. not passing a factory function), then you are also choosing not to use the inputs$
feature of the useObservable
hook and thus you are also not using the internal BehaviorSubject
.
If you try a hybrid approach, e.g. using a factory function that, internally, makes use of an observable created in outside context, then presumably that outside observable would be used inside a switchMap
or similar operator by the factory function, and this operator will ensure that the outside observable's subscription is completed on unsubscribe.
For example:
declare const otherObservable: Observable<boolean>;
useObservable(
(inputs$) =>
inputs$.pipe(
switchMap(([inputValue]) =>
return inputValue ? otherObservable : NEVER
)
)
)
I guess I don't know about mergeMap
though. I very rarely use mergeMap
so I'm not sure if it ever sends completed
events 🤔.
Root cause:
When using operators like
shareReplay()
, they typically rely on the source observable(i.e. theBehaviorSubject
insideuseObservable()
) to send acomplete
signal in order to unsubscribe the internalReplaySubject
from it. If nocomplete
signal is received, resource leaks could occur.Stackblitz URL: https://stackblitz.com/edit/stackblitz-starters-ghtjf1?devToolsHeight=33&file=src%2FApp.tsx
The example above is using RxJS 6. Haven't tested on RxJS 7 yet.
FantasyGauge
.FantasyGauge
we use a modified version ofuseObservable
withmyShareReplay
(which just logs some actions on top of originalshareReplay
logics from RxJS 6.2.1).FantasyGauge
is destroyed, the BehaviorSubject ofmyUseObservable
still holds someobservers
. See BEFORE.mp4 below.inputs$.complete()
, the BehaviorSubject no longer holds anyobservers
. See AFTER.mp4 below.I also find that
useObservableRef
anduseObservableCallback
both useBehaviorSubject
orSubject
under the hood, and both of them don't seem to callcomplete()
either. So I suspect these hooks might also be vulnerable to resource leaks.https://github.com/crimx/observable-hooks/assets/7220778/e8cca7a4-97ee-4ebf-94ce-a1d65eb609ed
https://github.com/crimx/observable-hooks/assets/7220778/ce66b77c-e8c0-41f7-9f4e-93627707529d