-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(core): allow unregistering callback through on
#11710
base: develop
Are you sure you want to change the base?
Conversation
While I can see where this is coming from, so far we decided against this to reduce complexity there. We generally do not do a lot of cleanup in a lot of places because (esp. in browser) because we make assumptions about the environment where something runs. I am not an Angular expect (cc @Lms24 ), but shouldn't the goal be to rather avoid the error handler to be continuously torn down and re-created? To me, it seems like this is the optimization that should be done, not to cleanup the one lone handler that is registered there. Now, I don't have a problem per-se to add this change, the main reason we do not have cleanup everywhere is because it adds bundle size (not necessarily this change itself, but ensuring everywhere that stuff is properly cleaned up can sum up to a not-so-small amount of code, sadly). Not saying we shouldn't do this, just thinking out loud... 🤔 |
Hey @mydea Well, it's generally considered good practice to have a cleanup functional API, akin to What you mentioned about the Angular error handler also applies to other frameworks that incorporate server-side rendering and could potentially extend to microfrontend applications. Take, for example, a React application that is rendered on the server side and destroyed with each HTTP request. Similarly, in the context of microfrontend applications, consider an app associated with a specific URL segment, such as |
Hey @arturovt thanks for opening this PR! Regarding the unsubscription mechanism we need to discuss this more thoroughly to decide what we're doing here. I'd tend to agree with @mydea that ideally we shouldn't add the extra bytes for correctly handling unsbscribing if at all possible. However, we should of course find a solution that doesn't cause hundreds of subscriptions from the errorHandler (or in other situations as you pointed out).
Is this really default behaviour in Angular? If so there's probably a good reason for it but on first glance it's a bit unintuitive/unnecessary to me 🤔 Would appreciate any context/knowledge around this, thx :) Also a general question: Is there a concrete reason for opening the PR? For example, did the SDK cause some kind of out of memory error that can be attributed to the |
@Lms24 This solution, lacking the functionality to remove an event listener when certain components are destroyed, such as any class with 'disposable' functionality, may inadvertently lead to memory leaks. When a callback captures local variables, there's no issue of leaks. However, if a callback captures Describing the technical necessity behind why this is crucial can be somewhat challenging, especially when explaining the importance of Considering an Angular example: when the root injector is destroyed, it also destroyes all injectees it maintains as records. You may observe a console.error coming from |
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.
We talked about this internally and decided to go ahead with this change. Thanks for explaining and of course for opening the PR! You're right, we should be good citizens and provide a cleanup functionality. Also obviously, use it in situations like the Angular error handler.
Before we can merge this, would you mind adding tests for the unregistering functionality?
@@ -176,52 +176,52 @@ export interface Client<O extends ClientOptions = ClientOptions> { | |||
* Register a callback for whenever a span is started. | |||
* Receives the span as argument. | |||
*/ | |||
on(hook: 'spanStart', callback: (span: Span) => void): void; | |||
on(hook: 'spanStart', callback: (span: Span) => void): () => void; |
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.
Let's add a comment to the JSDocs about what's returned from the methods
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.
I addressed your comments, I see there're some random failures happening between commits, not sure if they're related...
79ed42b
to
5a2041b
Compare
Hey, we just noticed that this kind of fell of the table in between reviews - sorry about that! Would you mind rebasing on develop and resolving any conflicts, then I think this should be ready for a final review & merge! |
5a2041b
to
1b8455c
Compare
This commit updates the return signature of the client's `on` function to be a void function, which, when executed, unregisters a callback. This adjustment is necessary for managing instances where objects are created and destroyed, ensuring that callbacks are properly unregistered to prevent self-referencing in callback closures and facilitate proper garbage collection. Typically, changing a type from `void` to `() => void` (or `VoidFunction`) shouldn't be considered a breaking change because `void` signifies the absence of a return value, implying that the return value of the `on` function should never be used by consumers. Opting for the `on` approach, which returns a cleanup function, "seems" simpler because having another function called `off` requires saving the callback reference for later removal. With our pattern, we encapsulate both the registration and removal of event listeners within a single function call.
1b8455c
to
87dd149
Compare
Done. |
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.
Nice, thank's a lot! Made some very minor adjustments to the tests, but LGTM 🚀
This commit updates the return signature of the client's
on
function to be a void function,which, when executed, unregisters a callback. This adjustment is necessary for managing instances
where objects are created and destroyed, ensuring that callbacks are properly unregistered to
prevent self-referencing in callback closures and facilitate proper garbage collection.
Typically, changing a type from
void
to() => void
(orVoidFunction
) shouldn't be considereda breaking change because
void
signifies the absence of a return value, implying that the returnvalue of the
on
function should never be used by consumers.Opting for the
on
approach, which returns a cleanup function, "seems" simpler because having anotherfunction called
off
requires saving the callback reference for later removal. With our pattern,we encapsulate both the registration and removal of event listeners within a single function call.