feat(compiler): add support for shorthand property declarations in templates #42421
Conversation
LGTM |
...i/test/compliance/test_cases/r3_view_compiler_template/GOLDEN_PARTIAL.js
Show resolved
Hide resolved
6e34a93
to
acd8d3f
acd8d3f
to
dd78667
I've addressed the span issue @JoostK. |
dd78667
to
bdcf9d9
Addressed the latest set of feedback and fixed the test failure. |
I think we should add more tests around each part of the system that supports the language service: The "template target" code ensures that we can map a position in a template to a target node in the TCB. This code is tested in
This brings up an immediate question: Should we just target the property read node via a We should add some tests around the TCB generation in After the template node is found, the language service builds a symbol for the template node. This part is tested in Lastly, we have specific functions in the language service for LSP operations (quick info, definitions, etc) using the template symbol information. Generally if the symbol was built correctly, the other parts should also work. Here's a test for quick info (
You propably want to add a similar test to |
4268143
to
c01bff2
...c/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
Outdated
Show resolved
Hide resolved
f8d61f1
to
a979077
The latest set of feedback has been addressed. |
@@ -129,6 +138,13 @@ describe('type check blocks diagnostics', () => { | |||
'(null as any ? (((ctx).a /*3,4*/) /*3,4*/)!.method /*6,12*/(((ctx).b /*13,14*/) /*13,14*/) : undefined) /*3,15*/'); | |||
}); | |||
|
|||
it('should annotate safe keyed reads', () => { |
alxhub
Jun 11, 2021
Contributor
This test appears unrelated to everything else in this PR. Is it here because prior designs broke this feature?
Either way, I would recommend extracting the addition of this test as a separate commit prior to the main commit, to avoid confusion.
This test appears unrelated to everything else in this PR. Is it here because prior designs broke this feature?
Either way, I would recommend extracting the addition of this test as a separate commit prior to the main commit, to avoid confusion.
crisbeto
Jun 12, 2021
•
Author
Member
These are tests that I missed in my safe keyed reads PR (#41911). I decided to add them here since this PR touches the same test files. I would rather keep them as is for now, because untangling them from the various fixup commits will take a while and may involve having to squash the commits.
These are tests that I missed in my safe keyed reads PR (#41911). I decided to add them here since this PR touches the same test files. I would rather keep them as is for now, because untangling them from the various fixup commits will take a while and may involve having to squash the commits.
alxhub
Jun 18, 2021
Contributor
I think we should squash the commits and then extract these tests into a separate commit, honestly.
It's not a big deal, but I think in the name of having a clean history, it makes sense not to bundle unrelated tests.
To avoid another round trip and blocking this PR, I'll go ahead and do that for you since I think it's ready to merge otherwise.
I think we should squash the commits and then extract these tests into a separate commit, honestly.
It's not a big deal, but I think in the name of having a clean history, it makes sense not to bundle unrelated tests.
To avoid another round trip and blocking this PR, I'll go ahead and do that for you since I think it's ready to merge otherwise.
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
Outdated
Show resolved
Hide resolved
If you wouldn't mind, I think we could also use a test here for a case like: <div #foo></div>
{{ m({foo}) }} just to verify that a shorthand reference to a property defined in the template itself still works. |
(and apologies for the last-minute change requests! this PR is |
97187a9
to
cd917ce
I've addressed the latest set of feedback @alxhub. |
cd917ce
to
97e8940
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the |
(I've pushed an update that splits out the extra tests for keyed reads into a separate commit, to keep the history clean) |
This commit adds some tests that were mistakenly omitted from the original change for safe keyed reads/writes.
…mplates Adds support for shorthand property declarations inside Angular templates. E.g. doing `{foo, bar}` instead of `{foo: foo, bar: bar}`. Fixes #10277.
97e8940
to
9847d6b
@AndrewKushnir suggests running a TGP, I will do so today. |
This CL is on the 1pm TAP train, should have results this afternoon. |
TGP is green. |
Adds support for shorthand property declarations inside Angular templates. E.g. writing
{foo, bar}
instead of{foo: foo, bar: bar}
.Fixes #10277.