8000 feat(compiler-cli): detect missing control flow directive imports by AndrewKushnir · Pull Request #46146 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

feat(compiler-cli): detect missing control flow directive imports #46146

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
Closed
wants to merge 1 commit into from
Closed

feat(compiler-cli): detect missing control flow directive imports #46146

wants to merge 1 commit into from

Conversation

AndrewKushnir
Copy link
Contributor

This commit adds an extended diagnostics check that verifies that all control flow directives (such as ngIf, ngFor) have matching directives. Currently there is no diagnistics produced for such cases, which makes it harder for developers to detect and find a problem.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added state: WIP area: compiler Issues related to `ngc`, Angular's template compiler target: rc This PR is targeted for the next release-candidate labels May 26, 2022
@AndrewKushnir AndrewKushnir requested a review from alxhub May 26, 2022 00:04
@ngbot ngbot bot modified the milestone: Backlog May 26, 2022
@AndrewKushnir AndrewKushnir force-pushed the error-message-improvement branch 2 times, most recently from 732c1c4 to 5ac5ecf Compare May 26, 2022 00:38
@AndrewKushnir AndrewKushnir added the cross-cutting: standalone Issues related to the NgModule-less world label May 26, 2022
@AndrewKushnir AndrewKushnir force-pushed the error-message-improvement branch from 5ac5ecf to d5d83a4 Compare May 26, 2022 02:07
@AndrewKushnir
Copy link
Contributor Author

Exploratory presubmit (TGP).

@Harpush
Copy link
Harpush commented May 26, 2022

Just wondering - but isn't this PR solving a specific case that should be solved by #37322 ? Once that one is fixed - ngIf without common module will be unknown structual directive like any other and error.

@pkozlowski-opensource
Copy link
Member

@Harpush you are right, it is a specific fix for a more general case described in #37322. It turns out, though, that having precise error message in a general case is not feasible as we don't have enough info about user's intention. This PR is a pragmatic compromise.

@Harpush
Copy link
Harpush commented May 26, 2022

@pkozlowski-opensource Shouldn't the error message (not from extended diagnostic) come from the compiler not finding the directive applied to the ng-emplate? I mean in both ngIf and any other structual directive the error is the same in case the directive isn't found (no error at all currently - Thus the linked issue). I agree that ngIf can be further extended to say the actual module that is missing.

@pkozlowski-opensource
Copy link
Member
pkozlowski-opensource commented May 26, 2022 •
edited
Loading

@Harpush right, suggestion about a NgModule to import is one aspect of it. When it comes to the "directive didn't match", the situation is more nuanced. #37322 talks about the specific situation: <div *something>. In compiler this will de-sugar to <ng-template something>.

Now the question we should ask: what is the meaning of something? It is reasonable to have 2 different takes on it:

  1. we expect that something is an attribute on which at least one directive should match - if there are no matching directives we should error - this is what you are suggesting;
  2. we assume that something is just a random attribute on which a directive can match - but is not required to. Such an attribute could be used for content projection, for example.

As we see the (2) usage in the wild we don't have much choice but, at this point, assume the (2) meaning. This is why we can't error on seeing <div *something> without a matching directive... I know that this is not ideal and might be confusing - but this is something we need to take into account when evolving a framework that has lots of existing code depending on it.

@Harpush
Copy link
Harpush commented May 26, 2022

@pkozlowski-opensource I understand the problem... Isn't there a way to "remember" it was once *something? If it was possible there would have been a certain distinction between random <ng-template something> and *something. That would solve the structual directives related problems when applied with *. Maybe even de-sugar to <ng-template [something]=""> or something similar.

@pkozlowski-opensource
Copy link
Member

@Harpush as I've mentioned we see usage of <div *someAttrForConentProjectionMatching> where people are trying to express <ng-template someAttrForConentProjectionMatching> without any intention of having a directive matching on someAttrForConentProjectionMatching. So this is not a question of remembering or any other technical solution - it is about the meaning / intention.

We could change the meaning to expect *directive to have a directive but this would be a breaking change.

@Harpush
Copy link
Harpush commented May 26, 2022

@pkozlowski-opensource I believe the current way some people use * is wrong... But it's indeed a breaking change and I am not sure it's a step you wish to take. A possible solution can be a compiler flag. We already have strict related compiler flags so why not add something like strictAttributeDirectives? Or a more deicated strictStructualDirectives?

@pkozlowski-opensource
Copy link
Member

I believe the current way some people use * is wrong...

Not sure if there is right / wrong here. I think that we just never made it intentional (in teaching / supported syntax / errors) so people can see it both ways.

At this point we are in the v14 RC period so a breaking change, even behind a compiler flag is not an option. And we would really need to dive deeper into desirable behaviour. Error message improvement is a temporary measure that we can do before the v14 final.

@AndrewKushnir AndrewKushnir force-pushed the error-message-improvement branch from d5d83a4 to ad6701b Compare May 26, 2022 18:54
@AndrewKushnir AndrewKushnir force-pushed the error-message-improvement branch 2 times, most recently from c1fb807 to 31ad317 Compare June 2, 2022 22:54
@AndrewKushnir AndrewKushnir force-pushed the error-message-improvement branch from 31ad317 to befd1bc Compare June 10, 2022 01:03
@AndrewKushnir AndrewKushnir added target: patch This PR is targeted for the next patch release and removed state: WIP target: rc This PR is targeted for the next release-candidate labels Jun 10, 2022
@AndrewKushnir AndrewKushnir marked this pull request as ready for review June 10, 2022 01:05
@AndrewKushnir
Copy link
Contributor Author
AndrewKushnir commented Jun 10, 2022 •
edited
Loading

Presubmit + TGP.

jessicajaniuk
jessicajaniuk approved these changes Jun 10, 2022
View reviewed changes
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.

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from dylhunn June 10, 2022 14:58
@pullapprove pullapprove bot requested a review from alxhub June 10, 2022 19:34
alxhub
alxhub approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member
@alxhub alxhub 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

@AndrewKushnir
feat(compiler-cli): detect missing control flow directive imports in …
66bdbb1 
…standalone components

This commit adds an extended diagnostics check that verifies that all control flow directives (such as `ngIf`, `ngFor`) have the necessary directives imported in standalone components. Currently there is no diagnostics produced for such cases, which makes it harder to detect and find problems.
@AndrewKushnir AndrewKushnir force-pushed the error-message-improvement branch from befd1bc to 66bdbb1 Compare June 10, 2022 19:39
@AndrewKushnir AndrewKushnir changed the title fix(compiler-cli): detect missing control flow directive imports feat(compiler-cli): detect missing control flow directive imports Jun 10, 2022
@AndrewKushnir AndrewKushnir added target: minor This PR is targeted for the next minor release action: merge The PR is ready for merge by the caretaker and removed target: patch This PR is targeted for the next patch release labels Jun 10, 2022
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 131d029.

@alxhub alxhub mentioned this pull request Jun 14, 2022
Open
@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 Jul 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alxhub alxhub alxhub 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 cross-cutting: standalone Issues related to the NgModule-less world target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
5 participants
@AndrewKushnir @Harpush @pkozlowski-opensource @jessicajaniuk @alxhub
0