Repository navigation
feat(angular): add DestroyRef support to subscribeWithPriority #31348
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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'; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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. | ||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||
| destroyRef?: DestroyRef | ||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this move to 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in /**
* @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 Both sentences are gone. I kept the second half of the |
||||||||||||||||||||||||||||||||||||||||||||||
| ): Subscription; | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Using The operator is only registering an Worth a line in the docs either way: passing an already-destroyed
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taken verbatim in I checked the tags rather than take it on trust:
We're on Your last paragraph is true of both kinds of That leaves the ordering. 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. |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing a
DestroyRefties this tongOnDestroytiming, and withion-router-outletthat 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
ionViewWillLeavefor 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.There was a problem hiding this comment.
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 noDestroyRefcallback 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
startHardwareBackButtonand a realbackbuttonevent: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.