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(service-worker): remove controllerchange listener when app is destroyed #55365

Closed

Conversation

arturovt
Copy link
Contributor

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.

@pullapprove pullapprove bot requested a review from alxhub April 16, 2024 16:12
@arturovt arturovt force-pushed the fix/service-worker-rm-controllerchange branch from 551c1c4 to 0cbfb5a Compare April 16, 2024 16:19
@pkozlowski-opensource pkozlowski-opensource added the area: service-worker Issues related to the @angular/service-worker package label Apr 16, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 16, 2024
@arturovt arturovt force-pushed the fix/service-worker-rm-controllerchange branch from 0cbfb5a to 2b09325 Compare April 17, 2024 16:53
@arturovt arturovt force-pushed the fix/service-worker-rm-controllerchange branch from 2b09325 to 299013b Compare May 19, 2024 11:46
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@arturovt thanks for the fix! The code looks great, just added 1 minor comment.

packages/service-worker/src/provider.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 20, 2024
@AndrewKushnir AndrewKushnir self-assigned this May 20, 2024
…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.
@arturovt arturovt force-pushed the fix/service-worker-rm-controllerchange branch from 299013b to aba98f7 Compare May 21, 2024 05:18
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels May 21, 2024
@AndrewKushnir
Copy link
Contributor

Caretaker note: this change is low-risk, no additional reviews are required, this PR is ready for merge.

@dylhunn dylhunn added target: rc This PR is targeted for the next release-candidate target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release target: rc This PR is targeted for the next release-candidate target: minor This PR is targeted for the next minor release labels May 21, 2024
@dylhunn
Copy link
Contributor

dylhunn commented May 21, 2024

It looks like this has merge conflicts with 17.3. I'm going to put it in v18 only; feel free to send a patch port if you want it in 17.3.x.

@dylhunn dylhunn removed the request for review from alxhub May 21, 2024 20:48
@dylhunn
Copy link
Contributor

dylhunn commented May 21, 2024

This PR was merged into the repository by commit afe4561.

dylhunn pushed a commit that referenced this pull request May 21, 2024
…estroyed (#55365)

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.

PR Close #55365
@dylhunn dylhunn closed this in afe4561 May 21, 2024
@arturovt arturovt deleted the fix/service-worker-rm-controllerchange branch May 21, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: service-worker Issues related to the @angular/service-worker package merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note PullApprove: disable target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants