-
Notifications
You must be signed in to change notification settings - Fork 6.8k
build: enable AoT compilation for all tests #31930
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
Conversation
Enables AoT compilation for all of our unit tests and fixes most of the errors that had accumulated over time.
}).toThrowError(/attempting to combine different/i); | ||
}); | ||
|
||
it('should throw an error if a toolbar-row is added later', () => { |
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.
This test wasn't doing anything. Also the error is being thrown inside an observable so we can't really catch it synchronously here.
it('should throw when trying to assign an invalid position', () => { | ||
expect(() => { | ||
fixture.componentInstance.position = 'everywhere'; | ||
fixture.componentInstance.position = 'everywhere' as 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.
Should this also be TooltipPosition?
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 test is checking what happens if an invalid position is passed in.
@if (true) { | ||
<mat-toolbar-row>First Row</mat-toolbar-row> | ||
} | ||
@if (true) { |
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.
Why can't we have both of these in the conditional like before?
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.
It's because the control flow syntax affects content projection.
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.
I still don't understand why they can't be in the same if (true) block. Seems a little unintuitive.
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.
Before we had the new control flow, if you'd written something like <foo *ngIf="true">
it would be de-sugared to <ng-template [ngIf]="true"><foo/></ng-template>
, however for content projection purposes Angular was copying the tag name and attributes of the foo
tag to the ng-template
tag. In order to avoid massive breaking changes, the new control flow does the same where the tag name/attributes will be copied onto the @if
node. The only limitation is that this copying only happens if there's a single root node inside the @if
. The compiler has a check that will flag cases where the @if
will prevent content projection which was happening in this test. I split the element across two separate @if
blocks so that they get content projected properly.
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.
Ah I see - thanks for the detailed explanation.
template: ` | ||
<input [matDatepicker]="d"> | ||
<mat-datepicker-toggle tabIndex="7" [for]="d" [disabled]="disabled"> | ||
<mat-datepicker-toggle [tabIndex]="7" [for]="d" [disabled]="disabled"> |
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.
What a weird tabIndex number. Should this just be 1?
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.
I think we were testing it with a weird number to make sure takes effect, as opposed to 1 or 0 which could be a default.
Enables AoT compilation for all of our unit tests and fixes most of the errors that had accumulated over time.