Skip to content
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

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

Merged
merged 10 commits into from
Oct 12, 2023

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Oct 12, 2023

Issue number: resolves #28337


What is the current behavior?

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:

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?

  • 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
  • No

Other information

Dev build: 7.5.1-dev.11697123035.1ee6b4a2

@github-actions github-actions bot added the package: angular @ionic/angular package label Oct 12, 2023
private menuController;

// TODO Now - Add type
constructor(private menuCtrl: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove the @Injectable from this class in common. We aren't using it with dependency injection in the extending implementations. If we did based on the current implementation, Angular would not know how to resolve menuCtrl and would throw an exception.

Since we are treating this as a base class of logic that is extended in the hydrated and custom elements implementations, I think we are safe to remove it.

re: Type, could you import the type from of menuController from @ionic/core/components? We already have some instances in the common package where we are assuming the CE build exists in the distributed package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed Injectable in base class in fb772c6

Re: the menuController type. We discussed offline, but the current controller interface was wrong so I fixed it in c102863

@github-actions github-actions bot added the package: core @ionic/core package label Oct 12, 2023
_register(menu: MenuI): void;
_unregister(menu: MenuI): void;

getMenus(): Promise<HTMLIonMenuElement[]>;
getOpenSync(): HTMLIonMenuElement | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

getOpenSync doesn't actually exist on the menu controller. However, _getOpenSync does so I think this was a typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

@liamdebeasi liamdebeasi marked this pull request as ready for review October 12, 2023 15:21
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe add a comment on the tests that they were created for issue 28337, like the annotations in core.

Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the quick turnaround on this!

@liamdebeasi liamdebeasi added this pull request to the merge queue Oct 12, 2023
Merged via the queue into main with commit fa78676 Oct 12, 2023
45 checks passed
@liamdebeasi liamdebeasi deleted the menu-controller-fix branch October 12, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: ionic angular 7.5, cannot access menus with menuController
4 participants