-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
refactor(angular): build with angular 16 #28902
Conversation
@@ -42,7 +42,9 @@ import { RouteView, StackDidChangeEvent, StackWillChangeEvent, getUrl, isTabSwit | |||
inputs: ['animated', 'animation', 'mode', 'swipeGesture'], | |||
}) | |||
// eslint-disable-next-line @angular-eslint/directive-class-suffix | |||
export class IonRouterOutlet implements OnDestroy, OnInit { | |||
export abstract class IonRouterOutlet implements OnDestroy, OnInit { | |||
abstract outletContent: any; |
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.
Previously we were passing in the outletContent via the constructor. However, newer versions of Angular warn that this will override the base implementation. However, this is what we want so I made the base class an abstract class to indicate that extending classes need to implement outletContent.
abstract outlet: any; | ||
abstract tabBar: any; | ||
abstract tabBars: any; |
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.
Newer versions of Angular warn that this will override the base implementation. However, this is what we want so I made the base class an abstract class to indicate that extending classes need to implement outlet, tabBar, and tabBars.
@Input() | ||
defaultHref: string | undefined; | ||
|
||
@Input() | ||
routerAnimation: AnimationBuilder | undefined; | ||
|
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 back button's inputs via the inputs
option in the extending classes decorator AND providing some properties using @input causes the latter to not be proxied to the Web Component. Additionally, this infrastructure isn't needed anyways because the type information for each property is already available.
@@ -296,7 +297,8 @@ export class IonRouterOutlet implements OnDestroy, OnInit { | |||
|
|||
// Calling `markForCheck` to make sure we will run the change detection when the | |||
// `RouterOutlet` is inside a `ChangeDetectionStrategy.OnPush` component. | |||
enteringView = this.stackCtrl.createView(this.activated, activatedRoute); | |||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | |||
enteringView = this.stackCtrl.createView(this.activated!, activatedRoute); |
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.
The build started to fail because this.activated
was potentially null
, but createView
does not accept null
for this param. This has always been a problem, but it only started to fail with Angular 16.
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.
LGTM, no issues locally. Thank you for the comments!
[Issue number: Internal
What is the current behavior?
Ionic 8 drops support for Angular 14 and 15. As a result, Ionic Angular should be built with Angular 16 to account for supported versions of Angular.
What is the new behavior?
Note: The ng14 and ng15 tests will fail because they are not compatible. These test apps are being removed as part of FW-5868. However, the rest of this PR can be reviewed. Once the test apps are removed, I will sync in the changes and all tests should pass.
Does this introduce a breaking change?
Other information