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.

Sign up for GitHub

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

Jump to bottom

fix(compiler-cli): implement more host directive validations as diagnostics #47768

Closed
wants to merge 1 commit into from
Closed

fix(compiler-cli): implement more host directive validations as diagnostics #47768

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

Implements more of the runtime validations for host directives as compiler diagnostics so that they can be caught earlier. Also does some minor cleanup.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: rc This PR is targeted for the next release-candidate labels Oct 14, 2022
@ngbot ngbot bot modified the milestone: Backlog Oct 14, 2022
crisbeto
crisbeto commented Oct 14, 2022
View reviewed changes
packages/compiler-cli/test/ngtsc/host_directives_spec.ts
@@ -357,8 +357,8 @@ runInEachFileSystem(() => {
import {ɵɵDirectiveDeclaration} from '@angular/core';

export declare class ExternalDir {
static ɵdir: ɵɵDirectiveDeclaration<ExternalDir, '[test]', never, never,
{input: "input"}, {output: "output"}, never, true, never>;
static ɵdir: ɵɵDirectiveDeclaration<ExternalDir, '[test]', never,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is fixing something I messed up when I first wrote the test. The new diagnostics picked up on it.

packages/compiler-cli/test/ngtsc/host_directives_spec.ts
@@ -434,7 +434,7 @@ runInEachFileSystem(() => {
diag.messageText.messageText;
}

it('should throw if a host directive is not standalone', () => {
it('should produce a diagnostic if a host directive is not standalone', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed all these tests talking about "throwing an error" to be more accurate.

packages/core/src/render3/features/host_directives_feature.ts
*/
function validateMappings(
bindingType: 'input'|'output', def: DirectiveDef<unknown>,
hostDirectiveDefs: HostDirectiveBindingMap) {
hostDirectiveBindings: HostDirectiveBindingMap) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This just renames the parameter to be more accurate.

crisbeto
crisbeto commented Oct 14, 2022
View reviewed changes
packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts
@@ -3072,52 +3072,6 @@ suppress
`Argument of type 'string' is not assignable to parameter of type 'number'.`
]);
});

it('should check bindings to host directive inputs referring to the private name when there is a different public name',
Copy link
Member Author

Choose a reason for hiding this comment

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

This test isn't valid anymore since it was testing what happens when trying to alias to the private name of an input/output.

@crisbeto
fix(compiler-cli): implement more host directive validations as diagn…
fc73369 
…ostics

Implements more of the runtime validations for host directives as compiler diagnostics so that they can be caught earlier. Also does some minor cleanup.
@crisbeto crisbeto force-pushed the additional-host-directive-diagnostics branch from b076d7a to fc73369 Compare October 14, 2022 08:01
@crisbeto crisbeto marked this pull request as ready for review October 14, 2022 08:21
benmed00
benmed00 reviewed Oct 14, 2022
View reviewed changes
packages/compiler-cli/src/ngtsc/annotations/common/src/diagnostics.ts
bindingType: 'input'|'output', hostDirectiveMeta: HostDirectiveMeta, meta: DirectiveMeta,
origin: ts.Expression, diagnostics: ts.DiagnosticWithLocation[]) {
const className = meta.name;
const hostDirectiveMappings =
Copy link

Choose a reason for hiding this comment

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

Why you don't explicitly declare the type, for ease of reading the code

Copy link
Member Author

Choose a reason for hiding this comment

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

Because TS can infer the type and the reader can hover over the name for more information.

jessicajaniuk
jessicajaniuk approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

reviewed-for: public-api, fw-core

AndrewKushnir
AndrewKushnir approved these changes Oct 14, 2022
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@pullapprove pullapprove bot requested a review from dylhunn October 14, 2022 16:36
@jessicajaniuk jessicajaniuk removed their request for review October 14, 2022 16:52
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 15, 2022
@crisbeto
Copy link
Member Author

Caretaker note: this PR was forced green for g3, because the only two failing targets were flakes. The g3 check status above doesn't reflect it, because we're in a transition period between two different ways of reporting the g3 status.

@dylhunn
Copy link
Contributor

dylhunn commented Oct 17, 2022

This PR was merged into the repository by commit f97bebf.

@dylhunn dylhunn closed this in f97bebf Oct 17, 2022
dylhunn pushed a commit that referenced this pull request Oct 17, 2022
@crisbeto @dylhunn
fix(compiler-cli): implement more host directive validations as diagn…
309b2cd 
…ostics (#47768)

Implements more of the runtime validations for host directives as compiler diagnostics so that they can be caught earlier. Also does some minor cleanup.

PR Close #47768
nouraellm pushed a commit to nouraellm/angular that referenced this pull request Nov 6, 2022
@crisbeto @nouraellm
fix(compiler-cli): implement more host directive validations as diagn…
a2a6d06 
…ostics (angular#47768)

Implements more of the runtime validations for host directives as compiler diagnostics so that they can be caught earlier. Also does some minor cleanup.

PR Close angular#47768
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benmed00 benmed00 benmed00 left review comments

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@crisbeto @dylhunn @benmed00 @AndrewKushnir @jessicajaniuk