GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat(compiler-cli): detect missing control flow directive imports by AndrewKushnir · Pull Request #46146 · angular/angular

Conversation

@AndrewKushnir

@AndrewKushnir AndrewKushnir commented May 26, 2022

Copy link
Copy Markdown
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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@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 commented May 26, 2022
View reviewed changes
Comment thread ...ges/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive/index.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@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

AndrewKushnir commented May 26, 2022

Copy link
Copy Markdown
Contributor Author

Exploratory presubmit (TGP).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Harpush

Harpush commented May 26, 2022

Copy link
Copy Markdown

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.

Brandinga reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pkozlowski-opensource

pkozlowski-opensource commented May 26, 2022

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Harpush

Harpush commented May 26, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pkozlowski-opensource

pkozlowski-opensource commented May 26, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Brandinga reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Harpush

Harpush commented May 26, 2022

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pkozlowski-opensource

pkozlowski-opensource commented May 26, 2022

Copy link
Copy Markdown
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 reacted with confused emoji
All reactions
  • 😕 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Harpush

Harpush commented May 26, 2022

Copy link
Copy Markdown

@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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@pkozlowski-opensource

pkozlowski-opensource commented May 26, 2022

Copy link
Copy Markdown
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.

Harpush reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
alxhub
alxhub reviewed Jun 3, 2022
View reviewed changes
Comment thread packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

alxhub
alxhub approved these changes Jun 3, 2022
View reviewed changes
Comment thread ...k/extended/test/checks/missing_control_flow_directive/missing_control_flow_directive_spec.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

@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
@pullapprove
pullapprove Bot requested review from alxhub, atscott and jessicajaniuk June 10, 2022 01:05
@AndrewKushnir

AndrewKushnir commented Jun 10, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Presubmit + TGP.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jessicajaniuk
jessicajaniuk approved these changes Jun 10, 2022
View reviewed changes

@jessicajaniuk jessicajaniuk left a comment

Copy link
Copy Markdown
Contributor

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.

reviewed-for: public-api

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@pullapprove
pullapprove Bot requested a review from dylhunn June 10, 2022 14:58
alxhub
alxhub approved these changes Jun 10, 2022
View reviewed changes
@pullapprove
pullapprove Bot requested a review from alxhub June 10, 2022 19:34
alxhub
alxhub approved these changes Jun 10, 2022
View reviewed changes

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

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.

Reviewed-for: public-api

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@AndrewKushnir
AndrewKushnir removed request for atscott and dylhunn June 10, 2022 19:35
@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

jessicajaniuk commented Jun 10, 2022

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jessicajaniuk jessicajaniuk closed this in 131d029 Jun 10, 2022
@alxhub alxhub mentioned this pull request Jun 14, 2022
Missing standalone components declared with button[custom-button] are not reported by the compiler #46351
Open
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Jul 11, 2022

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
+1 more reviewer
@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@AndrewKushnir @Harpush @pkozlowski-opensource @jessicajaniuk @alxhub
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.