ionic-team / ionic-team/ionic-framework

feat(angular): DestroyRef support for subscribeWithPriority

Abierto
#31,366 0 comentarios 0 reacciones 0 asignados Ver en GitHub
triage
Lenguaje dominante
TypeScript
Estrellas
52.7k
Forks
13.3k
Merge medio
1 d 15 h
PR fusionados (30 d)
51

Descripción

### Prerequisites

- [X] I have read the [Contributing Guidelines](https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#creating-an-issue).
- [X] I agree to follow the [Code of Conduct](https://ionicframework.com/code-of-conduct).
- [X] I have searched for [existing issues](https://github.com/ionic-team/ionic-framework/issues) that already include this feature request, without success.

### Describe the Feature Request

Add an optional third argument to `BackButtonEmitter.subscribeWithPriority`, a `DestroyRef`, so the subscription ends when the scope that opened it is destroyed:

```ts
this.platform.backButton.subscribeWithPriority(10, (processNext) => { ... }, inject(DestroyRef));
```

Omitted, behaviour is unchanged.

> **Correction to the first version of this issue.** It claimed that three visits to a page make one back press run the handler three times. That number came from a stand-in dispatcher in my own harness which ignored priority and invoked every registered handler; Ionic runs one handler per press, as [the documentation](https://ionicframework.com/docs/developing/hardware-back-button) says plainly. The real consequence is worse than a duplicate run and is described below. Two other corrections: the docs page has eight `subscribeWithPriority` calls across six constructor examples, not seven; and #30115, cited previously, is not evidence for this — see the end of the use case.

### Describe the Use Case

`Platform` is `providedIn: 'root'`, so `backButton` is one application-lifetime `Subject`. A handler registered from a page stays registered after that page is destroyed, and nothing in the emitter removes it.

Combined with the documented selection rules, that goes further than an extra call. From the hardware back button page:

> By default, only one handler is fired per hardware back button press.

> In the event that there are handlers with the same priority value, the handler that was registered _last_ will be called.

So take the ordinary case. Page A is on screen and registered a handler at priority 10. The user pushes B, which registers one at priority 10 too, and pops back to A. B is destroyed; B's subscription is not, and it was registered after A's. The next press is a tie, the tie goes to the handler registered last, and that is B's — a page the user has already left. **A's handler never runs, and the press is consumed by a destroyed page.**

Measured with Ionic's own dispatcher rather than a stand-in this time: released `@ionic/core`'s `startHardwareBackButton`, the real `Platform`, a jsdom document and a synchronous `NgZone` stand-in, and a press is a real `backbutton` event on `document`, which is what core listens for and what it turns into the `ionBackButton` event carrying the real `register`.

```
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)
```

Rows three and four are the same defect without needing a tie: a page that registered a higher priority keeps outranking whatever is on screen for the rest of the session.

The rows with a `DestroyRef` run the proposed function itself — the change is an assignment to `backButton.subscribeWithPriority` inside `Platform`'s constructor, so the harness assigns exactly that body to the real `Subject`. The rows without it go through an installed 8.8.3 that carries the change locally, but with no third argument its path is `source$ = this` followed by the same `subscribe`, so it behaves as released code on that path. Saying which build produced which row matters here, and the first version of this issue did not.

Two things I should be straight about:

- **This is already solvable.** The control row is the workaround that exists today: keep the `Subscription` and unsubscribe in `ngOnDestroy`. It works. The request is about ergonomics and about what the documentation teaches, not about capability.
- **Not every revisit adds a subscription.** A page still in the stack is reattached rather than reconstructed — `StackController.getExistingView` calls `changeDetectorRef.reattach()` — so a second subscription needs the page to be constructed again, which happens after it has been popped, or after a root navigation.

What makes it worth changing rather than documenting alone: the signature takes a callback and returns something most callers ignore, and the [hardware back button page](https://ionicframework.com/docs/developing/hardware-back-button) has eight `subscribeWithPriority` calls inside six constructor examples and does not mention cleanup once. The documented pattern is the one that accumulates, and the symptom — a back press handled by a page that is gone — is silent and gets worse with use.

On [#30115](https://github.com/ionic-team/ionic-framework/issues/30115), which I cited in the first version of this issue: it does not support this. That reporter keeps the `Subscription` and unsubscribes in `ngOnDestroy`, which is the control row above, and still sees a duplicate. It is evidence that this API confuses people, not evidence of the accumulation this parameter would prevent, and I should not have filed it under the use case.

### Describe Preferred Solution

An optional `DestroyRef` parameter, with the teardown registered directly rather than through an operator:

```ts
subscribeWithPriority(
priority: number,
callback: (processNextHandler: () => void) => Promise | void,
destroyRef?: DestroyRef
): Subscription;
```

```ts
const subscription = this.subscribe((ev) => {
return ev.register(priority, (processNextHandler) => zone.run(() => callback(processNextHandler)));
});

destroyRef?.onDestroy(() => subscription.unsubscribe());

return subscription;
```

`DestroyRef` has been `@publicApi` since Angular 16, so it is within this package's `>=16` floor. `takeUntilDestroyed` would not be: it is `@developerPreview` in 16 and still in 18, and only `@publicApi` from 19.

One consequence to note either way: `subscribeWithPriority` can then throw, where today it never does. For a component's `DestroyRef`, registering on an already-destroyed view throws `VIEW_ALREADY_DESTROYED`.

**One scope limit worth settling here rather than in a pull request.** A `DestroyRef` ties to `ngOnDestroy`, and with `ion-router-outlet` that runs when a page is popped, not when it is navigated away from — `StackController.cleanup()` destroys only the views that have left the stack and detaches the others. So this ends handlers that outlive their page. It does nothing for a page still in the stack whose handler can win a press while another page is on screen; that needs the enter and leave lifecycle hooks, and no lifetime-based API can address it. Those are two different problems, and if the team would rather have one visibility-aware mechanism than a lifetime one, that is a reasonable answer and better decided now.

### Describe Alternatives

- **Keep the `Subscription` and unsubscribe manually.** Works today. Easy to omit, and the omission is invisible until a back press goes to the wrong page.
- **Document the cleanup instead of changing the API.** Worth doing regardless of the outcome here, since the current examples teach the shape that accumulates, but it leaves every caller writing the same boilerplate.
- **A separate method** such as `subscribeWithPriorityUntil`, leaving the existing signature untouched.
- **Have `Platform` track subscriptions itself.** Rejected: it cannot know which scope a caller belongs to.

### Related Code

Minimal page:

```ts
export class LeakyPage implements OnInit {
private readonly platform = inject(Platform);

ngOnInit() {
this.platform.backButton.subscribeWithPriority(10, (processNext) => {
console.log('handler ran');
processNext();
});
}
}
```

Push to it from a page that registers its own handler at the same priority, pop back, then press the hardware back button: the log comes from the page that is gone.

The pull request will add an executable version of this to `packages/angular/test/base/e2e`, beside the existing `back-button.spec.ts`, dispatching a `backbutton` event on `document` the way `core/src/utils/test/hardware-back-button.spec.ts` does — hand-dispatching `ionBackButton` would supply a fake `detail.register` and prove nothing, which is the mistake the corrected measurement above avoids.

### Additional Information

Pull request: [#31348](https://github.com/ionic-team/ionic-framework/pull/31348).

The change has been running in production in an Ionic 8 app of ours, applied to the built `fesm2022` output, which is what prompted submitting it upstream.

Guía de contribución

Abrir la guía de contribución

Línea de trabajo

Start at BackButtonEmitter.subscribeWithPriority and compare the requested signature and teardown behavior with the existing implementation. Add the executable case under packages/angular/test/base/e2e beside back-button.spec.ts, using core/src/utils/test/hardware-back-button.spec.ts as the event-dispatch reference. Done means destroyed-page handlers no longer win, while omitted DestroyRef behavior remains unchanged.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
angular, typescript
Área
api, frontend
Tipo de issue
Nueva funcionalidad
Dificultad
3/5
Tiempo estimado
1-2 días
Estado de actividad
Estancado
Claridad
Bien especificado
Aptitud para principiantes
35/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.