Skip to content

feat(angular): add DestroyRef support to subscribeWithPriority - #31348

Open
MaximBelov wants to merge 1 commit into
ionic-team:mainfrom
MaximBelov:back-button-subscribe-destroyref
Open

feat(angular): add DestroyRef support to subscribeWithPriority#31348
MaximBelov wants to merge 1 commit into
ionic-team:mainfrom
MaximBelov:back-button-subscribe-destroyref

Conversation

@MaximBelov

Copy link
Copy Markdown

Issue number: resolves #


What is the current behavior?

BackButtonEmitter.subscribeWithPriority gives you no way to stop listening.

Platform is providedIn: 'root', so backButton is a single application-lifetime Subject. A component that registers a handler:

this.platform.backButton.subscribeWithPriority(10, (processNext) => {
  this.closeMyThing();
  processNext();
});

keeps that handler registered after it is destroyed. The callback keeps running — against a destroyed component — for as long as the app is alive, and every navigation to that page adds another one. The only way out today is to hold the Subscription and unsubscribe by hand in ngOnDestroy, which is easy to forget and impossible to notice going wrong: nothing errors, the handler just quietly still runs.

What is the new behavior?

  • subscribeWithPriority takes an optional third argument, a DestroyRef. When it is passed, the stream is piped through takeUntilDestroyed, so the subscription ends with the component that opened it:

    this.platform.backButton.subscribeWithPriority(10, (processNext) => { ... }, inject(DestroyRef));
  • Omit it and behaviour is byte-for-byte what it is today, so nothing existing changes.

  • takeUntilDestroyed is handed an explicit DestroyRef rather than relying on an injection context — that is what makes it usable from the assignment inside the zone.run callback, which is not one.

Does this introduce a breaking change?

  • Yes
  • No

The parameter is optional and the no-argument path is unchanged. takeUntilDestroyed needs Angular 16, and this package already declares @angular/core: >=16.0.0, so it is within the supported range. It is the first use of @angular/core/rxjs-interop in the repository.

Other information

On testing. packages/angular's test script is echo 'angular no tests yet' and there are no unit specs in the package, so there is nothing here to extend — test/ holds e2e apps. I did not add a unit-test harness to the package, since introducing one is a much larger change than this fix and not mine to decide. What I did instead:

  • Type-checked the change under --strict against real @angular/core and rxjs typings, including the this.pipe(...) : this union that subscribe is called on.
  • The equivalent change has been running in production in an Ionic 8 app of ours, applied to the built fesm2022 output with patch-package, which is what prompted submitting it properly.

Happy to shape it differently — an overload instead of an optional parameter, or a separate method — if either fits the API better.

subscribeWithPriority had no way to stop listening. The Platform it lives on
is provided in root, so a subscription taken in a component outlives that
component and keeps firing its callback after the component is gone -- for
the lifetime of the application.

Adds an optional destroyRef parameter. When passed, the stream is piped
through takeUntilDestroyed so the subscription ends with the component that
opened it. Omitted, behaviour is byte-for-byte what it was.

takeUntilDestroyed is given an explicit DestroyRef rather than relying on an
injection context, which is what makes it usable from the assignment inside
the zone.run callback.
@MaximBelov
MaximBelov requested a review from a team as a code owner August 12, 2026 08:56
@vercel

vercel Bot commented Aug 12, 2026

Copy link
Copy Markdown

@MaximBelov is attempting to deploy a commit to the Ionic Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions Bot added the package: angular @ionic/angular package label Aug 12, 2026
@ShaneK ShaneK changed the title fix(angular): allow back button subscriptions to be tied to a DestroyRef feat(angular): add DestroyRef support to subscribeWithPriority Aug 16, 2026

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for putting this together, and for the thorough write-up!

Before the code though, one process thing. This is really a feat rather than a fix, since it adds an optional parameter to a public signature rather than correcting existing behavior, so I've retitled it to feat(angular): add DestroyRef support to subscribeWithPriority to match what it's doing. All community PRs need an associated issue with a clear use case, and feature PRs go through our internal design process before we can merge. The description has Issue number: resolves # with nothing after it and I couldn't find an existing issue covering this, so could you open one? A small repro showing the handler still firing after the page is gone would help a lot.

A few other things from my pass:

  • I left a question inline about which lifetime this ties to. I'm not sure it covers the scenario your description opens with, and that's worth settling before going further on the implementation.
  • This needs a companion docs PR. Our hardware back button page shows subscribeWithPriority in a constructor in a few places and never mentions cleanup, so the documented pattern is the one that leaks. That's arguably the higher impact half of this.
  • You're right that packages/angular has no unit harness and I wouldn't ask you to add one. There's a Playwright app at packages/angular/test/base though, and since core dispatches ionBackButton on document, a page that registers a handler, navigates away, and asserts it stopped firing would work there.
  • Small correction on the description: the explicit DestroyRef isn't needed because of the zone.run assignment. The operator runs when the method is called, not when it's assigned, so the assignment site doesn't come into it. It's needed because we can't assume the caller's context.

Heads up that this file moved to packages/angular/src/common/providers/platform.ts on major-9.0 - which will definitely deploy before this PR can be merged, so it won't cherry-pick and will need a hand-port. It's targeting a minor either way, so no rush on your end.

Comment on lines +15 to +19
/**
* Pass a component's `DestroyRef` to have the subscription torn down with that
* component. Without it the subscription lives for the lifetime of the injector,
* which for a root-provided `Platform` means the lifetime of the application.
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Passing a DestroyRef ties this to ngOnDestroy timing, and with ion-router-outlet that only fires when a page is popped. Pushing from A to B leaves A in the DOM, so A's handler stays subscribed while B is on screen. That's the case your description opens with, and I don't think this would fix it.

What do you see when you push a few pages deep with this applied? I haven't run your app so I could be wrong about how it plays out in practice.

Our Angular lifecycle docs point people at ionViewWillLeave for unsubscribing for this reason. If that's the right hook, then "torn down with that component" is going to read as covering navigate-away when it doesn't.

Comment on lines +73 to 79
this.backButton.subscribeWithPriority = function (priority, callback, destroyRef) {
const source$ = destroyRef ? this.pipe(takeUntilDestroyed(destroyRef)) : this;

return source$.subscribe((ev) => {
return ev.register(priority, (processNextHandler) => zone.run(() => callback(processNextHandler)));
});
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
this.backButton.subscribeWithPriority = function (priority, callback, destroyRef) {
const source$ = destroyRef ? this.pipe(takeUntilDestroyed(destroyRef)) : this;
return source$.subscribe((ev) => {
return ev.register(priority, (processNextHandler) => zone.run(() => callback(processNextHandler)));
});
};
this.backButton.subscribeWithPriority = function (priority, callback, destroyRef) {
const subscription = this.subscribe((ev) => {
return ev.register(priority, (processNextHandler) => zone.run(() => callback(processNextHandler)));
});
destroyRef?.onDestroy(() => subscription.unsubscribe());
return subscription;
};

Using takeUntilDestroyed means depending on developer preview API. It's @developerPreview in Angular 16, still is in 18, and only becomes @publicApi in 19, so neither our >=16 floor here nor >=18 on major-9.0 gets the stable version. The DestroyRef type itself has been stable since 16 though.

The operator is only registering an onDestroy and using its unregister function as teardown, so doing that directly gets the same behavior and drops the @angular/core/rxjs-interop import, which would be our first runtime import from a secondary @angular/core entry point.

Worth a line in the docs either way: passing an already-destroyed DestroyRef will now throw, where before this method never threw at all.

* component. Without it the subscription lives for the lifetime of the injector,
* which for a root-provided `Platform` means the lifetime of the application.
*/
destroyRef?: DestroyRef

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this move to @param tags in one block above the signature? That's the shape we use elsewhere, swipeGesture in the menu controller being the closest example, and it uses the [menuId] bracket form for the optional one.

I'd also trim the last two sentences. They explain why the change exists rather than what the parameter does, and they're already in the commit body and the PR description, so that's three copies to keep in sync. Up to you on that one.

@ShaneK ShaneK added the needs: reply the issue needs a response from the user label Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs: reply the issue needs a response from the user package: angular @ionic/angular package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants