GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat(compiler): report unknown property bindings on explicit ng-templa… by aparzi · Pull Request #69640 · angular/angular Conversation
|
|
This PR introduce a little Breaking Change. Existing code that has unused properties in directives will receive a compiler error. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Presubmit came back green. I'll have to run a TGP to see how many breakages it triggers. This will help us determine how we can land this change. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Let me know how the TGP looks. I'm ready to help with any adjustments or schematics if it triggers some internal breakages. Thanks! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yeah having a schematic to help with the cleanup would be great. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@JeanMeche |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
The TGP raised (at least) a false positive: Throws with your changes while Another simpler one that reports a false positive: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
f5a5d21 to
1d4a41f
Compare
|
There is another false positive that gets flaged:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
1d4a41f to
0ae017a
Compare
Good catch, thanks! [foobar] matches the directive's selector even without a corresponding input, so it's valid usage that shouldn't be flagged. I've updated the check to only report bindings on an when no directive matched the node at all, this still covers the missing-import scenario from #69322, while tolerating selector-only bindings like this one. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
0ae017a to
20b6d22
Compare
20b6d22 to
89fad3d
Compare
5f8cf15 to
4f6ab48
Compare
There was an error while loading. Please reload this page.
| expect(diags.length).toBe(0); | ||
| }); | ||
|
|
||
| it('should not report unclaimed properties on an ng-template that matches a directive', () => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why?
Ah, it's in the comments in TCB generation:
If any directive matched, the node is not checked, since a binding may exist solely to trigger directive matching via its selector.
That's really unfortunate. Perhaps we should be cross-checking against the selector, or simpler: treat the presence of a binding value/expression as requiring a corresponding input. A missing input means the bound value is always disregarded, so may be reported as error. That leaves only bare attributes without value as unreported even if it doesn't correspond with a selector (if we don't cross-check against the selector).
I don't know if we should go as far as checking against the selector, but I'd prefer we do if we can come up with an appropriate check.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The force push including a rebase makes GH's "Compare" mode useless so it's now very hard to spot what has changed to this extent. What's the current state of this? I think I may have seen updated tests and parsing a selector, but I also still see the "a binding may exist solely to trigger directive matching" comment.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I dont remember all the details either. The check is now should report unclaimed properties on an ng-template that matches a directive.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| expect(diags.map((d) => d.messageText)).toEqual([]); | ||
| }); | ||
|
|
||
| it('should not report properties on an ng-template inside an svg element', () => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Is the ng-template treated as svg:ng-template here (inheriting the implicit SVG namespace) for this to not act like ng-template? Shouldn't that report a diagnostic of its own (when custom element schema is not present).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is still pending
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here. Would you expect a separate diagnostic if we hit a svg:ng-template ? What would the diagnostic expose ?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This test captures behavior where the newly introduced diagnostic doesn't fire for svg:ng-template, because supposedly that isn't an actual ng-template. But then I'd indeed expect an unknown element diagnostic for this scenario.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| ); | ||
| }); | ||
|
|
||
| describe('strictNgTemplateTypes', () => { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked, but does removing the isExplicitNgTemplate condition cause any test failures? I suppose it will, but if not we'd need a test for that (i.e. structural directive syntax)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
should not report properties on an ng-template inside an svg element is failing when isExplicitNgTemplate only returns true.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The question was whether there's an explicit structural directive test as well, outside of the SVG namespace.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This is still an open question
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
bcbdae2 to
baeffad
Compare
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@aparzi This needs a rebase and has conflicts that need resolving. Can you take a look? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
reviewed-for: public-api
Sorry, something went wrong.
There was an error while loading. Please reload this page.
yes |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
738837a to
55860b3
Compare
|
@thePunderWoman I think @JeanMeche did a rebase |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
55860b3 to
49205c8
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@aparzi @JeanMeche Looks like this is conflicted again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
49205c8 to
7dc15b4
Compare
…ate elements Previously, property bindings on `<ng-template>` that were not claimed by a matched directive were silently ignored by the type checker. This was inconsistent with how bindings on regular elements are handled. This check is added behind a temporary compiler flag `checkTypeOfNgTemplateBindings` which will be removed in the next breaking change window. When the flag is enabled, the type checker will report an error for any property binding on an `<ng-template>` that is not matched by a directive. Fixes angular#69322 Co-authored-by: Matthieu Riegler <[email protected]>
7dc15b4 to
39a5e0d
Compare
There was an error while loading. Please reload this page.
|
This PR was merged into the repository. The changes were merged into the following branches:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Compiler doesn't report missing import for a directive applied onng-template
There was an error while loading. Please reload this page.