Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions packages/angular/common/src/providers/platform.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { DOCUMENT } from '@angular/common';
import { NgZone, Inject, Injectable } from '@angular/core';
import type { DestroyRef } from '@angular/core';
import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
import { getPlatforms, isPlatform } from '@ionic/core/components';
import type { BackButtonEventDetail, KeyboardEventDetail, Platforms } from '@ionic/core/components';
import { Subscription, Subject } from 'rxjs';
Expand All @@ -9,7 +11,13 @@ import { Subscription, Subject } from 'rxjs';
export interface BackButtonEmitter extends Subject<BackButtonEventDetail> {
subscribeWithPriority(
priority: number,
callback: (processNextHandler: () => void) => Promise<any> | void
callback: (processNextHandler: () => void) => Promise<any> | void,
/**
* 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right. cleanup() only destroys views that have left the stack, so pushing A → B never destroys A and no DestroyRef callback fires.

I got a second thing wrong too: I said A's handler "still fires". It usually doesn't. One handler wins per press, and a tie goes to whichever registered last.

That makes the symptom worse than I described. Push A → B, pop back to A. B is gone but its subscription isn't, and it registered after A's, so the next press goes to B and A's handler never runs. Measured with startHardwareBackButton and a real backbutton event:

A on screen, destroyed B still subscribed        -> B (destroyed)
same, but B was given a DestroyRef               -> A (on screen)
destroyed page at priority 20, live page at 10   -> destroyed page (20)
same, with a DestroyRef on the destroyed one     -> live page (10)
control -- B unsubscribed by hand instead        -> A (on screen)

I haven't pushed a few pages deep in a real app with this applied, so I won't claim I have. Those rows are what I ran.

So this fixes handlers outliving their page. It can't fix a page in the stack winning a press while another is on screen — that needs the leave hook and the enter hook, because a page coming back off the stack is only reattached and never re-subscribes. Which of the two you'd rather have is the open question in #31366, and I'd like your call before I go further.

Both mistakes are fixed elsewhere as well: the abridged cleanup() I quoted here is gone, and #31366 no longer claims a press runs the handler three times. That number came from a dispatcher stub of mine that ignored priority.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done in be7f5381:

/**
 * @param priority  Handlers with a higher priority run first.
 * @param callback  Called with a function that passes control to the next handler.
 * @param [destroyRef]  Optionally unsubscribe when this `DestroyRef` is destroyed. For a page
 * in an `ion-router-outlet` that is when the page is popped off the stack, not when it is
 * navigated away from.
 * @returns the subscription, which can also be unsubscribed by hand.
 */

Tag style and bracket form are from MenuController, though I wouldn't claim it follows swipeGesture structurally — those are class methods, this is an interface member, and there's no precedent for a block on one here. @returns matches the two tags platform.ts already has.

Both sentences are gone. I kept the second half of the destroyRef line because of your other comment: without it, "unsubscribe when this DestroyRef is destroyed" reads as covering navigate-away. Happy to move it to the docs page if you'd rather.

): Subscription;
}

Expand Down Expand Up @@ -62,8 +70,10 @@ export class Platform {
constructor(@Inject(DOCUMENT) private doc: any, zone: NgZone) {
zone.run(() => {
this.win = doc.defaultView;
this.backButton.subscribeWithPriority = function (priority, callback) {
return this.subscribe((ev) => {
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)));
});
};
Comment on lines +75 to 83

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Taken verbatim in 50e467ab.

I checked the tags rather than take it on trust:

Angular takeUntilDestroyed DestroyRef
16.0.0 @developerPreview @publicApi
18.0.0 @developerPreview @publicApi
19.0.0 @publicApi @publicApi

We're on >=16.0.0 here and >=18.0.0 on major-9.0, so neither floor gets the stable operator.

Your last paragraph is true of both kinds of DestroyRef, not just one. A component's throws VIEW_ALREADY_DESTROYED, one from an environment injector throws INJECTOR_ALREADY_DESTROYED. Same on 16 through 20. I've put that in the description.

That leaves the ordering. onDestroy runs after subscribe, so if the DestroyRef is already destroyed the subscription gets registered and then the call throws — the caller sees an exception and the handler is left subscribed with nothing to remove it.

So either we document that the argument has to be live, like you suggested, or:

try {
  destroyRef?.onDestroy(() => subscription.unsubscribe());
} catch (e) {
  subscription.unsubscribe();
  throw e;
}

The commit is your snippet unchanged, so right now it's the first. Just say if you'd rather it couldn't leave a subscription behind.

Also — I measured on Angular 21, not 16. The floor claim rests on the tag, not on a run.

Expand Down