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.

Sign up for GitHub

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

Jump to bottom

fix(service-worker): remove controllerchange listener when app is destroyed #56001

Closed
wants to merge 1 commit into from
Closed

fix(service-worker): remove controllerchange listener when app is destroyed #56001

wants to merge 1 commit into from

Conversation

arturovt
Copy link
Contributor

No description provided.

@pullapprove pullapprove bot requested a review from alxhub May 22, 2024 17:05
@arturovt
Copy link
Contributor Author

@dylhunn this is the same as #55365, but targets 17.3.x branch.

@arturovt
fix(service-worker): remove controllerchange listener when app is d…
c6f0de2 
…estroyed

This commit updates the `ngswAppInitializer` implementation and removes the `controllerchange`
listener upon the destruction of the `ApplicationRef`. This adjustment aims to prevent memory
leaks. In a zone.js environment, neglecting to do so could lead to the perpetual creation of
a zone task, which captures the zone and obstructs proper garbage collection.
@dylhunn dylhunn added the area: service-worker Issues related to the @angular/service-worker package label May 22, 2024
@ngbot ngbot bot added this to the Backlog milestone May 22, 2024
@JeanMeche
Copy link
Member

17.3.x is now a LTS branch, are we sure we want to ship it there ?

@dylhunn
Copy link
Contributor

dylhunn commented May 22, 2024

Yeah, 17.3.x is now LTS. We can do it if you like, but I recommend against it unless you have a real reason to backport it.

@dylhunn
Copy link
Contributor

dylhunn commented May 22, 2024 •
edited

Oh actually, it looks like I put the original PR on main, which is 18.1.x, not 18.0.x (which was rc, now patch). Our branch configuration is really weird during the RC, and I got a bit confused. Sorry about that! We can port to 18.0.1 if you like. Regardless, I'd still advise against LTS ports.

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented May 22, 2024 •
edited

Regardless, I'd still advise against LTS ports.

+1, let's avoid merging this fix into an LTS branch and land it into the 18.0.x branch (and release it in a patch version). @arturovt could you please update the PR to target 18.0.x branch instead?

@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels May 22, 2024
@JeanMeche
Copy link
Member

@AndrewKushnir It already landed in the RC.

@AndrewKushnir
Copy link
Contributor

It already landed in the RC.

@JeanMeche thanks for additional info, I'll close the PR since it has landed in RC already.

@arturovt arturovt deleted the 17.3.x branch May 23, 2024 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alxhub alxhub Awaiting requested review from alxhub

Assignees
No one assigned
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: service-worker Issues related to the @angular/service-worker package target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@arturovt @JeanMeche @dylhunn @AndrewKushnir