Skip to content
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

fix(angular): Remove afterSendEvent listener once root injector is destroyed #12786

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

arturovt
Copy link
Contributor

@arturovt arturovt commented Jul 5, 2024

In this commit, we added cleanup logic to handle the removal of afterSendEvent, which is set up within the Angular error handler. This fixes memory leaks that occur when the event is still being handled after the root view is removed.

@arturovt arturovt force-pushed the fix/angular-rm-listener branch from 1c1690e to 3bb8cc8 Compare July 5, 2024 16:13
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @arturovt thanks for opening this PR!

The fix makes sense to me so I'm happy to merge this. Any chance you could add a small test to cover this? We already have some tests for showReportDialog in ErrorHandler so something similar to these would be great:

describe('opens the report dialog if `showDialog` is true', () => {

(Just a heads-up: I'm only assigning myself to the PR for internal tracking since I'm reviewing it)

@Lms24 Lms24 self-assigned this Jul 8, 2024
@Lms24 Lms24 changed the title fix(angular): remove afterSendEvent listener once root injector is destroyed fix(angular): Remove afterSendEvent listener once root injector is destroyed Jul 8, 2024
mydea added a commit that referenced this pull request Jul 11, 2024
Small optimization using the new hook cleanup capabilities to remove
unused hooks.

Ref PR to do this in angular:
#12786
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some tests to cover the cleanup logic since I had some time and I think we should get this out sooner than later.

Thanks for contributing once again ❤️

arturovt and others added 2 commits July 18, 2024 13:50
…destroyed

In this commit, we added cleanup logic to handle the removal of `afterSendEvent`,
which is set up within the Angular error handler. This fixes memory leaks that occur
when the event is still being handled after the root view is removed.
@Lms24 Lms24 force-pushed the fix/angular-rm-listener branch from eae93f2 to ed22329 Compare July 18, 2024 11:50
@Lms24 Lms24 enabled auto-merge (squash) July 18, 2024 11:51
@Lms24 Lms24 merged commit 7500b06 into getsentry:develop Jul 18, 2024
87 checks passed
andreiborza added a commit that referenced this pull request Jul 18, 2024
This PR adds the external contributor to the CHANGELOG.md file, so that
they are credited for their contribution. See #12786

---------

Co-authored-by: Lms24 <8420481+Lms24@users.noreply.github.com>
Co-authored-by: Andrei Borza <andrei.borza@sentry.io>
@arturovt arturovt deleted the fix/angular-rm-listener branch July 18, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants