feat(compiler): add support for shorthand property declarations in templates #42421
Conversation
There was a problem hiding this comment.
The change generally looks good, with a comment on parse spans.
I think this syntax requires special attention in the language service when renaming a property, as the shorthand syntax may need to be expanded in that case. @atscott WDYT about this case?
packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_template/GOLDEN_PARTIAL.js
Show resolved
Hide resolved
6e34a93 to
acd8d3f
Compare
acd8d3f to
dd78667
Compare
|
I've addressed the span issue @JoostK. |
dd78667 to
bdcf9d9
Compare
|
Addressed the latest set of feedback and fixed the test failure. |
There was a problem hiding this comment.
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 packages/language-service/ivy/test/legacy/template_target_spec.ts (note that these tests do not pass so there needs to be an update to the template target code):
describe('object literal shorthand', () => {
fit('with one item', () => {
debugger;
const {errors, nodes, position} = parse(`{{ {va¦l1} }}`);
expect(errors).toBe(null);
const {context} = getTargetAtPosition(nodes, position)!;
expect(context.kind).toBe(TargetNodeKind.RawExpression);
// Maybe this should be multinode target so we target both the key and the value of the object
// literal map
const {node} = context as SingleNodeTarget;
expect(node).toBeInstanceOf(e.PropertyRead);
expect((node as e.PropertyRead).name).toBe('val1');
});
fit('with two shorthands', () => {
const {errors, nodes, position} = parse(`{{ {val1, va¦l2} }}`);
expect(errors).toBe(null);
const {context} = getTargetAtPosition(nodes, position)!;
expect(context.kind).toBe(TargetNodeKind.RawExpression);
const {node} = context as SingleNodeTarget;
expect(node).toBeInstanceOf(e.PropertyRead);
expect((node as e.PropertyRead).name).toBe('val2');
});
fit('with shorthand and regular syntax', () => {
const {errors, nodes, position} = parse(`{{ {val1: 'val1', va¦l2} }}`);
expect(errors).toBe(null);
const {context} = getTargetAtPosition(nodes, position)!;
expect(context.kind).toBe(TargetNodeKind.RawExpression);
const {node} = context as SingleNodeTarget;
expect(node).toBeInstanceOf(e.PropertyRead);
expect((node as e.PropertyRead).name).toBe('val2');
});
});
This brings up an immediate question: Should we just target the property read node via a SingleNodeTarget? The way the TCB is currently generated, I think that's all we can do.
We should add some tests around the TCB generation in packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts. This is the second part of the question above: What do we do about the divergent representations in the type check block vs what's in the template? The template code is written as {val1} and then generated as {"val1": ((ctx).val1 /*span*/) /*span*/} in the TCB. The language service leverages the TCB for most of the actions we perform. Because the TCB output is different than the representation in the template, it would be hard to get everything fully correct (renames, go to definition, etc.). This might just be something we have to be okay with and just do actions off of the property read. We might need to add a new template target type that can detect this case and not allow renaming. Or we just allow a rename from the property read and be okay that we get it wrong where template shorthand is used (which hopefully isn't many places and at least the compiler will detect them and complain).
After the template node is found, the language service builds a symbol for the template node. This part is tested in packages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts. Please add a test there to ensure we build the correct symbol for this case.
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 (packages/language-service/ivy/test/quick_info_spec.ts):
// TODO(atscott): This should be able to be added to the previous section as long as the project
// has a different name than the one in the beforeEach but something goes wrong in the test
// environment
describe('non-skeleton project', () => {
beforeEach(() => {
initMockFileSystem('Native');
env = LanguageServiceTestEnv.setup();
});
fit('should work for shorthand object literal', () => {
project = env.addProject(
'test', {
'app.ts': `
import {Component, NgModule} from '@angular/core';
import {CommonModule} from '@angular/common';
@Component({
selector: 'some-cmp',
templateUrl: './app.html',
})
export class SomeCmp {
val1 = 'one';
val2 = 2;
doSomething(obj: {val1: string, val2: number}) {
}
}
@NgModule({
declarations: [SomeCmp],
imports: [CommonModule],
})
export class AppModule{
}
`,
'app.html': `{{doSomething({val1, val2})}}`,
},
{strictTemplates: true});
env.expectNoSourceDiagnostics();
project.expectNoSourceDiagnostics();
const template = project.openFile('app.html');
template.moveCursorToText('val¦1');
const quickInfo = template.getQuickInfoAtPosition();
expect(toText(quickInfo!.displayParts)).toEqual('(property) val1: string');
template.moveCursorToText('val¦2');
const quickInfo2 = template.getQuickInfoAtPosition();
expect(toText(quickInfo!.displayParts)).toEqual('(property) val2: number');
});
});
You propably want to add a similar test to definitions_spec in the same folder to ensure "go to definition" works properly as well as the references_and_rename_spec.
4268143 to
c01bff2
Compare
...ages/compiler-cli/src/ngtsc/typecheck/test/type_checker__get_symbol_of_template_node_spec.ts
Outdated
Show resolved
Hide resolved
f
8000
8d61f1 to
a979077
Compare
|
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', () => { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍 otherwise, this is just a confusing part of the system and i want to make sure it's as clear as possible for people in the future) |
97187a9 to
cd917ce
Compare
|
I've addressed the latest set of feedback @alxhub. |
cd917ce to
97e8940
Compare
|
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 ℹ️ Googlers: Go here for more info. |
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 angular#10277.
97e8940 to
9847d6b
Compare
|
@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. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adds support for shorthand property declarations inside Angular templates. E.g. writing
{foo, bar}instead of{foo: foo, bar: bar}.Fixes #10277.