feat(angular): add DestroyRef support to subscribeWithPriority - #31348
feat(angular): add DestroyRef support to subscribeWithPriority#31348MaximBelov wants to merge 3 commits into
Conversation
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 is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
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
subscribeWithPriorityin 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/angularhas no unit harness and I wouldn't ask you to add one. There's a Playwright app atpackages/angular/test/basethough, and since core dispatchesionBackButtonondocument, a page that registers a handler, navigates away, and asserts it stopped firing would work there. - Small correction on the description: the explicit
DestroyRefisn't needed because of thezone.runassignment. 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.
| /** | ||
| * 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. | ||
| */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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))); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
| * 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
From review: takeUntilDestroyed is @developerPreview in Angular 16 and 18, and only @publicapi from 19, so this package's >=16 floor never gets the stable operator. DestroyRef has been @publicapi since 16. Registering onDestroy directly behaves the same and drops the @angular/core/rxjs-interop import.
Tag style and the bracket form for the optional parameter follow MenuController; @returns matches the two tags platform.ts already has. The destroyRef entry says which lifetime it follows, since a page in an ion-router-outlet is destroyed on pop rather than on navigating away.
Issue number: resolves #31366
What is the current behavior?
BackButtonEmitter.subscribeWithPrioritygives you no declarative way to stop listening, and the documented examples never do it by hand.PlatformisprovidedIn: 'root', sobackButtonis a single application-lifetimeSubject. A page that registers a handler:keeps it registered after the page is destroyed. Each visit that constructs the page again adds another one — which happens after the page has been popped, or after a root navigation; a page still in the stack is reattached rather than rebuilt.
The consequence is not a duplicate call. Only one handler runs per press, and a tie goes to whichever was registered last, so after pushing to a page and popping back, the press is won by the handler belonging to the page that is gone, and the on-screen page's handler never runs. Measured with
@ionic/core's ownstartHardwareBackButtonand a realbackbuttonevent:The control row is the workaround available today: hold the
Subscriptionand unsubscribe inngOnDestroy. It works, so this is about ergonomics and about what the documentation teaches — #31366 has the full case, including the counts on the docs page.What is the new behavior?
subscribeWithPrioritytakes an optional third argument, aDestroyRef, and registers the teardown on it directly:Omit it and the path is unchanged, so nothing existing changes.
Which lifetime this is. A
DestroyReffires withngOnDestroy, and underion-router-outletthat is when a page is popped off the stack, not when it is navigated away from. So this ends handlers that outlive their page. It does not silence a page that is still in the stack while another is on screen; that needs the enter and leave lifecycle hooks, and no lifetime-based API can do it. Raised in feat(angular): DestroyRef support for subscribeWithPriority #31366 as a design question, since a visibility-aware mechanism would be a different feature.Does this introduce a breaking change?
The parameter is optional and the no-argument path is unchanged.
DestroyRefhas been@publicApisince Angular 16, and this package declares@angular/core: >=16.0.0, so it is within the supported range.One behavioural note that is new rather than breaking:
subscribeWithPrioritycan now throw, where before it never did. Either implementation ofDestroyRefthrows if it is already destroyed —VIEW_ALREADY_DESTROYEDfor a component's,INJECTOR_ALREADY_DESTROYEDfor one taken from an environment injector.Other information
Two commits: the maintainer's suggested form applied on top of the original change, then the
@paramblock on its own.On testing. A Playwright spec belongs in
packages/angular/test/base/e2e/src/standalone/, beside the existingback-button.spec.ts, and it is coming: a page that registers a handler with and without aDestroyRef, popped, then abackbuttonevent dispatched ondocument— the waycore/src/utils/test/hardware-back-button.spec.tsdoes it, since hand-dispatchingionBackButtonsupplies a fakedetail.registerand proves nothing. An earlier version of this description said there was nowhere in the package to put a test, which was simply wrong.A docs pull request is also needed and is not written yet: the hardware back button page shows this API in six constructor examples and mentions cleanup in none of them, which is arguably the more useful half of the change.