feat(compiler-cli): detect missing structural directive imports #59443
Conversation
There was a problem hiding this comment.
LGTM, thanks for the PR.
The diagnostic is named control flow, but might be fine. Also adding @thePunderWoman as the original author IIRC
Yeah that was one of the things I was unsure about. Would it be better as a separate diagnostic? Would renaming this diagnostic be considered a breaking change? |
|
@manbearwiz Since this extended diagnostic is specifically about control flow, this seems out of scope to me. I think it would be better to have a separate optional template diagnostic for this since it doesn't apply to everyone. What do you think? |
|
That works for me. I'll update it to be a separate diagnostic. Any other suggestions while I'm in there? This will definitely touch a lot more files |
8000
f0b2991 to
9cdc812
Compare
|
@manbearwiz It looks like you have legitimate test failures. Can you take a look? |
ee7a298 to
c46f119
Compare
c46f119 to
8e5faa2
Compare
|
G3 has been cleaned up. We'll go through a re-review after the recent changes. |
...check/extended/test/checks/missing_structural_directive/missing_structural_directive_spec.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
Outdated
Show resolved
Hide resolved
...ages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
Show resolved
Hide resolved
...ages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
Outdated
Show resolved
Hide resolved
...ages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
Outdated
Show resolved
Hide resolved
a6f82c3 to
17216d5
Compare
17216d5 to
c76c88b
Compare
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
Outdated
Show resolved
Hide resolved
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken |
|
Passing TGP, the broken target got fixed in between. |
|
This PR was merged into the repository by commit c889382. The changes were merged into the following branches: main |
|
@manbearwiz Thanks for your effort into this PR! 💯 |
|
Of course! Glad it got in. Thanks @JeanMeche for taking it the rest of the way! |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #37322
Currently, the compiler will warn about missing directive imports if they are one of the "known" structural directives from the CommonModule or use regular binding syntax. This extends that to include custom, user-defined, structural directives. Currently, missing an import for these custom structural directives just fails silently which is a pain point when doing standalone migrations.
What is the new behavior?
The compiler will now warn on missing custom structural directives.
Does this PR introduce a breaking change?
I made minimal changes and left existing logic in order to ensure the changes are not breaking. I am definitely open to taking another approach, adding more test cases, etc.