fix(angular): do not create duplicate menuController instances (#28343)

Issue number: resolves #28337

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

Duplicate instances of `menuController` are being created in
`@ionic/angular`. `ion-menu` registers itself in the `menuController`
from `@ionic/core`, but the `MenuController` from `@ionic/angular` uses
the `menuController` from `@ionic/core/components`. This is how the
overlay providers work too. Normally, this is not a problem. However,
`menuController` caches references to registered menus in each
controller instances:
https://github.com/ionic-team/ionic-framework/blob/dcbf45101f4c0dc3541cd4c19186daa3766a6ce7/core/src/utils/menu-controller/index.ts#L14

This means that since there are two different controllers,
`menuController` B does not know about the menus in `menuController` A.
The end result is that the menu controller used in developer
applications did not have references to the registered menus, which gave
the impression that the menu controller did not work.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Updated the architecture of `MenuController` in Ionic Angular to
accept a `menuController` instance. This allows `@ionic/angular` to pass
the `menuController` from `@ionic/core` and for
`@ionic/angular/standalone` to pass the `menuController` from
`@ionic/core/components`.

Note: Overlay controllers don't **need** this change per-se since they
don't cache references to overlays internally (they just query the DOM).
However, I think it would be good to have a consistent architecture
here, so I'll put up a separate PR that makes this change for overlays
too.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

Dev build: `7.5.1-dev.11697123035.1ee6b4a2`

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Author: liamdebeasi

core/src/components/menu/menu-interface.ts

@@ -1,4 +1,4 @@
-import type { Animation } from '../../interface';
+import type { Animation, AnimationBuilder } from '@utils/animation/animation-interface';
 
 export type Side = 'start' | 'end';
 
@@ -26,13 +26,23 @@ export interface MenuI {
 }
 
 export interface MenuControllerI {
+  registerAnimation(name: string, animation: AnimationBuilder): void;
+  get(menu?: string | null, logOnMultipleSideMenus?: boolean): Promise<HTMLIonMenuElement | undefined>;
+  getMenus(): Promise<HTMLIonMenuElement[]>;
+  getOpen(): Promise<HTMLIonMenuElement | undefined>;
+  isEnabled(menu?: string | null): Promise<boolean>;
+  swipeGesture(shouldEnable: boolean, menu?: string | null): Promise<HTMLIonMenuElement | undefined>;
+  isAnimating(): Promise<boolean>;
+  isOpen(menu?: string | null): Promise<boolean>;
+  enable(shouldEnable: boolean, menu?: string | null): Promise<HTMLIonMenuElement | undefined>;
+  toggle(menu?: string | null): Promise<boolean>;
+  close(menu?: string | null): Promise<boolean>;
+  open(menu?: string | null): Promise<boolean>;
+  _getOpenSync(): HTMLIonMenuElement | undefined;
   _createAnimation(type: string, menuCmp: MenuI): Promise<Animation>;
-  _setOpen(menu: MenuI, shouldOpen: boolean, animated: boolean): Promise<boolean>;
   _register(menu: MenuI): void;
   _unregister(menu: MenuI): void;
-
-  getMenus(): Promise<HTMLIonMenuElement[]>;
-  getOpenSync(): HTMLIonMenuElement | undefined;
+  _setOpen(menu: MenuI, shouldOpen: boolean, animated: boolean): Promise<boolean>;
 }
 
 export interface MenuChangeEventDetail {

core/src/utils/menu-controller/index.ts

@@ -1,6 +1,6 @@
 import { printIonWarning } from '@utils/logging';
 
-import type { MenuI } from '../../components/menu/menu-interface';
+import type { MenuI, MenuControllerI } from '../../components/menu/menu-interface';
 import type { AnimationBuilder, BackButtonEvent } from '../../interface';
 import { MENU_BACK_BUTTON_PRIORITY } from '../hardware-back-button';
 import { componentOnReady } from '../helpers';
@@ -9,7 +9,7 @@ import { menuOverlayAnimation } from './animations/overlay';
 import { menuPushAnimation } from './animations/push';
 import { menuRevealAnimation } from './animations/reveal';
 
-const createMenuController = () => {
+const createMenuController = (): MenuControllerI => {
   const menuAnimations = new Map<string, AnimationBuilder>();
   const menus: MenuI[] = [];
 

packages/angular/common/src/providers/menu-controller.ts

@@ -1,17 +1,15 @@
-import { Injectable } from '@angular/core';
-import { menuController } from '@ionic/core/components';
+import type { MenuControllerI } from '@ionic/core/components';
 
-@Injectable({
-  providedIn: 'root',
-})
 export class MenuController {
+  constructor(private menuController: MenuControllerI) {}
+
   /**
    * Programmatically open the Menu.
    * @param [menuId]  Optionally get the menu by its id, or side.
    * @return returns a promise when the menu is fully opened
    */
   open(menuId?: string): Promise<boolean> {
-    return menuController.open(menuId);
+    return this.menuController.open(menuId);
   }
 
   /**
@@ -22,7 +20,7 @@ export class MenuController {
    * @return returns a promise when the menu is fully closed
    */
   close(menuId?: string): Promise<boolean> {
-    return menuController.close(menuId);
+    return this.menuController.close(menuId);
   }
 
   /**
@@ -32,7 +30,7 @@ export class MenuController {
    * @return returns a promise when the menu has been toggled
    */
   toggle(menuId?: string): Promise<boolean> {
-    return menuController.toggle(menuId);
+    return this.menuController.toggle(menuId);
   }
 
   /**
@@ -44,7 +42,7 @@ export class MenuController {
    * @return Returns the instance of the menu, which is useful for chaining.
    */
   enable(shouldEnable: boolean, menuId?: string): Promise<HTMLIonMenuElement | undefined> {
-    return menuController.enable(shouldEnable, menuId);
+    return this.menuController.enable(shouldEnable, menuId);
   }
 
   /**
@@ -54,7 +52,7 @@ export class MenuController {
    * @return Returns the instance of the menu, which is useful for chaining.
    */
   swipeGesture(shouldEnable: boolean, menuId?: string): Promise<HTMLIonMenuElement | undefined> {
-    return menuController.swipeGesture(shouldEnable, menuId);
+    return this.menuController.swipeGesture(shouldEnable, menuId);
   }
 
   /**
@@ -63,15 +61,15 @@ export class MenuController {
    * If the menuId is not specified, it returns true if ANY menu is currenly open.
    */
   isOpen(menuId?: string): Promise<boolean> {
-    return menuController.isOpen(menuId);
+    return this.menuController.isOpen(menuId);
   }
 
   /**
    * @param [menuId]  Optionally get the menu by its id, or side.
    * @return Returns true if the menu is currently enabled, otherwise false.
    */
   isEnabled(menuId?: string): Promise<boolean> {
-    return menuController.isEnabled(menuId);
+    return this.menuController.isEnabled(menuId);
   }
 
   /**
@@ -84,20 +82,20 @@ export class MenuController {
    * @return Returns the instance of the menu if found, otherwise `null`.
    */
   get(menuId?: string): Promise<HTMLIonMenuElement | undefined> {
-    return menuController.get(menuId);
+    return this.menuController.get(menuId);
   }
 
   /**
    * @return Returns the instance of the menu already opened, otherwise `null`.
    */
   getOpen(): Promise<HTMLIonMenuElement | undefined> {
-    return menuController.getOpen();
+    return this.menuController.getOpen();
   }
 
   /**
    * @return Returns an array of all menu instances.
    */
   getMenus(): Promise<HTMLIonMenuElement[]> {
-    return menuController.getMenus();
+    return this.menuController.getMenus();
   }
 }

packages/angular/src/index.ts

@@ -23,7 +23,6 @@ export {
   ActionSheetController,
   AlertController,
   LoadingController,
-  MenuController,
   ModalController,
   PickerController,
   PopoverController,
@@ -42,6 +41,7 @@ export {
   ViewDidEnter,
   ViewDidLeave,
 } from '@ionic/angular/common';
+export { MenuController } from './providers/menu-controller';
 
 // PACKAGE MODULE
 export { IonicModule } from './ionic-module';

packages/angular/src/providers/menu-controller.ts

@@ -0,0 +1,12 @@
+import { Injectable } from '@angular/core';
+import { MenuController as MenuControllerBase } from '@ionic/angular/common';
+import { menuController } from '@ionic/core';
+
+@Injectable({
+  providedIn: 'root',
+})
+export class MenuController extends MenuControllerBase {
+  constructor() {
+    super(menuController);
+  }
+}

packages/angular/standalone/src/index.ts

@@ -5,11 +5,11 @@ export { IonRouterOutlet } from './navigation/router-outlet';
 export { IonRouterLink, IonRouterLinkWithHref } from './navigation/router-link-delegate';
 export { IonTabs } from './navigation/tabs';
 export { provideIonicAngular } from './providers/ionic-angular';
+export { MenuController } from './providers/menu-controller';
 export {
   ActionSheetController,
   AlertController,
   LoadingController,
-  MenuController,
   ModalController,
   PickerController,
   PopoverController,

packages/angular/standalone/src/providers/menu-controller.ts

@@ -0,0 +1,12 @@
+import { Injectable } from '@angular/core';
+import { MenuController as MenuControllerBase } from '@ionic/angular/common';
+import { menuController } from '@ionic/core/components';
+
+@Injectable({
+  providedIn: 'root',
+})
+export class MenuController extends MenuControllerBase {
+  constructor() {
+    super(menuController);
+  }
+}

packages/angular/test/base/e2e/src/lazy/providers.spec.ts

@@ -27,5 +27,11 @@ describe('Providers', () => {
 
     cy.get('#query-params').should('have.text', 'firstParam: abc, firstParam: true');
   })
+
+  // https://github.com/ionic-team/ionic-framework/issues/28337
+  it('should register menus correctly', () => {
+    cy.get('#set-menu-count').click();
+    cy.get('#registered-menu-count').should('have.text', '1');
+  });
 });
 

packages/angular/test/base/e2e/src/standalone/menu-controller.spec.ts

@@ -0,0 +1,11 @@
+describe('Menu Controller', () => {
+  beforeEach(() => {
+    cy.visit('/standalone/menu-controller');
+  })
+
+  // https://github.com/ionic-team/ionic-framework/issues/28337
+  it('should register menus correctly', () => {
+    cy.get('#set-menu-count').click();
+    cy.get('#registered-menu-count').should('have.text', '1');
+  });
+})

packages/angular/test/base/src/app/lazy/providers/providers.component.html

@@ -1,39 +1,48 @@
+<ion-menu menu-id="menu" content-id="content">
+  Menu Content
+</ion-menu>
+
 <ion-header>
-    <ion-toolbar>
-      <ion-title>
-        Providers Test
-      </ion-title>
-    </ion-toolbar>
-  </ion-header>
-  <ion-content class="ion-padding">
-    <p>
-      isLoaded: <span id="is-loaded">{{isLoaded}}</span>
-    </p>
-    <p>
-      isReady: <span id="is-ready">{{isReady}}</span>
-    </p>
-    <p>
-      isResumed: <span id="is-resumed">{{isResumed}}</span>
-    </p>
-    <p>
-      isPaused: <span id="is-paused">{{isPaused}}</span>
-    </p>
-    <p>
-      isResized: <span id="is-resized">{{isResized}}</span>
-    </p>
-    <p>
-      isTesting: <span id="is-testing">{{isTesting}}</span>
-    </p>
-    <p>
-      isDesktop: <span id="is-desktop">{{isDesktop}}</span>
-    </p>
-    <p>
-      isMobile: <span id="is-mobile">{{isMobile}}</span>
-    </p>
-    <p>
-      keyboardHeight: <span id="keyboard-height">{{keyboardHeight}}</span>
-    </p>
-    <p>
-      queryParams: <span id="query-params">{{queryParams}}</span>
-    </p>
-  </ion-content>
+  <ion-toolbar>
+    <ion-title>
+      Providers Test
+    </ion-title>
+  </ion-toolbar>
+</ion-header>
+<ion-content class="ion-padding" id="content">
+  <p>
+    isLoaded: <span id="is-loaded">{{isLoaded}}</span>
+  </p>
+  <p>
+    isReady: <span id="is-ready">{{isReady}}</span>
+  </p>
+  <p>
+    isResumed: <span id="is-resumed">{{isResumed}}</span>
+  </p>
+  <p>
+    isPaused: <span id="is-paused">{{isPaused}}</span>
+  </p>
+  <p>
+    isResized: <span id="is-resized">{{isResized}}</span>
+  </p>
+  <p>
+    isTesting: <span id="is-testing">{{isTesting}}</span>
+  </p>
+  <p>
+    isDesktop: <span id="is-desktop">{{isDesktop}}</span>
+  </p>
+  <p>
+    isMobile: <span id="is-mobile">{{isMobile}}</span>
+  </p>
+  <p>
+    keyboardHeight: <span id="keyboard-height">{{keyboardHeight}}</span>
+  </p>
+  <p>
+    queryParams: <span id="query-params">{{queryParams}}</span>
+  </p>
+  <p>
+    Registered Menu Count: <span id="registered-menu-count">{{registeredMenuCount}}</span>
+  </p>
+
+  <button id="set-menu-count" (click)="setMenuCount()">Set Menu Count</button>
+</ion-content>