Fix edge case issue around unsubscribing live queries #4868
Conversation
| @@ -69,7 +67,7 @@ hook useLazyLoadQueryNode<TQuery: OperationType>({ | |||
| fetchObservable, | |||
| fetchPolicy, | |||
| renderPolicy, | |||
| {start: startFetch, complete: completeFetch, error: completeFetch}, | |||
| undefined, | |||
There was a problem hiding this comment.
Could you help me understand why these are safe to remove?
There was a problem hiding this comment.
This was for calling .unsubscribe() when the component unmounts. However since the unsubscribe is now a no-op there's no point for keeping this anymore
| }, | ||
| }; | ||
| observerStart(subscriptionWithConditionalCancelation); | ||
| unsubscribe: () => {}, |
There was a problem hiding this comment.
Do live queries still get unsubscribed through some other means?
There was a problem hiding this comment.
Yeah it's managed by the .retain() stuff and is also ensured with the test added (the .toThrow() part)
XiNiHa
force-pushed
the
fix-live-unsubscribe
branch
from
0b4cf67 to
b7dc565
Compare
|
@XiNiHa I think I've been hesitant to merge this for fear that there are some edge cases I'm not aware of here and that it could break things when I roll it out. Would you be willing to add this change (fix) behind a feature flag? |
|
@XiNiHa Curious if you'll be able to add a flag here. |
|
Oops, missed the notification 😅 Will make it done today |
XiNiHa
force-pushed
the
fix-live-unsubscribe
branch
from
b7dc565 to
2890ce1
Compare
XiNiHa
force-pushed
the
fix-live-unsubscribe
branch
from
2890ce1 to
3a012df
Compare
XiNiHa
force-pushed
the
fix-live-unsubscribe
branch
from
3a012df to
2a9058d
Compare
|
The test started to pass even without the fix 🤔 let me figure out what's happening... |
|
@XiNiHa Any progress here? |
|
I haven't looked into this for a while. Maybe we could just close the PR since it wasn't possible to reproduce the issue after rebasing.... |
This only happens when...
useLazyLoadQueryNode()The fix is pretty simple though 😋