Skip to content

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

Closed
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:additional-host-directive-diagnostics
Closed

fix(compiler-cli): implement more host directive validations as diagnostics#47768
crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto:additional-host-directive-diagnostics

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
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.

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.

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.

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.

…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
@pullapprove pullapprove bot requested review from JoostK and jessicajaniuk October 14, 2022 08:21
bindingType: 'input'|'output', hostDirectiveMeta: HostDirectiveMeta, meta: DirectiveMeta,
origin: ts.Expression, diagnostics: ts.DiagnosticWithLocation[]) {
const className = meta.name;
const hostDirectiveMappings =

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.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

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

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
…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
@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.

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

Development

Successfully merging this pull request may close these issues.

5 participants