-
-
Notifications
You must be signed in to change notification settings - Fork 177
Cancel requests when element is removed #1045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
} | ||
|
||
export type ActionContext = RuntimeContext & { | ||
setCleanup: (fn: OnRemovalFn, key: string) => void // sets a cleanup function for this element and key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since actions can return values to callers, I needed to add a new context API that allow setting a cleanup based on a key. The key can't be auto-set to the action name because there could be different fetch actions used on the same element and we only want to keep the removalFn for the last fetch action.
I faced a problem today (see Discord thread) and it seems this change is addressing it. I wanted to bind an SSE connection to a particular view, and once that view is morphed away or replaced the SSE connection would reset. Currently it just keeps creating more SSE connections on every page transition. I kept wondering why my app is bugging out, turns out it was the multiple SSE connections patch-racing. |
@wmTJc9IK0Q Yes that is similar to my use case. I have an SSE endpoint that powers a specific server-side component and pushes updates to it. Whenever the parent component removes this child component I want the requests to stop and no more patching of elements. |
Maybe, for reasons I currently don't see, it makes sense to allow the old behavior through an option still:
Detaching means it won't get canceled if But it definitely makes sense to make cancelation the new default behavior. If you want to keep an SSE stream triggered by an element, keep that element. Current behavior is rather counterintuitive and potentially dangerous (memory leaks, patch races). |
Thanks for the PR. We need to discuss internally if and how we want to handle this. |
@bencroker Anything I can do to move this forward? |
Not really. We've been busy and haven't come to a decision on this yet. |
This makes it so that when requestCancellation is
auto
and the element that initiated the request is removed, the request will now be cancelled. Otherwise open SSE requests keep getting new events and patching elements that shouldn't be and resources are not released causing memory leaks.