The Wayback Machine - https://web.archive.org/web/20210625005813/http://github.com/angular/angular/pull/42421
Skip to content
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.

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

feat(compiler): add support for shorthand property declarations in templates #42421

Closed

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented May 30, 2021

Adds support for shorthand property declarations inside Angular templates. E.g. writing {foo, bar} instead of {foo: foo, bar: bar}.

Fixes #10277.

@google-cla google-cla bot added the cla: yes label May 30, 2021
@crisbeto crisbeto marked this pull request as ready for review May 30, 2021
@ngbot ngbot bot modified the milestone: Backlog May 30, 2021
@pullapprove pullapprove bot requested review from jessicajaniuk and JoostK May 30, 2021
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

LGTM 🍪

Copy link
Member

@JoostK JoostK left a 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?

@crisbeto crisbeto force-pushed the crisbeto:10277/property-shorthand-declaration branch from 6e34a93 to acd8d3f Jun 2, 2021
@crisbeto crisbeto force-pushed the crisbeto:10277/property-shorthand-declaration branch from acd8d3f to dd78667 Jun 2, 2021
@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Jun 2, 2021

I've addressed the span issue @JoostK.

@crisbeto crisbeto force-pushed the crisbeto:10277/property-shorthand-declaration branch from dd78667 to bdcf9d9 Jun 2, 2021
@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Jun 2, 2021

Addressed the latest set of feedback and fixed the test failure.

Copy link
Contributor

@atscott atscott left a 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.

@crisbeto crisbeto force-pushed the crisbeto:10277/property-shorthand-declaration branch 2 times, most recently from 4268143 to c01bff2 Jun 6, 2021
@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Jun 6, 2021

@atscott all of the feedback should be covered in the latest fixup commit. I've also included more tests for safe keyed reads since I didn't get the chance to add them before #41911 got merged in.

@crisbeto crisbeto force-pushed the crisbeto:10277/property-shorthand-declaration branch 2 times, most recently from f8d61f1 to a979077 Jun 7, 2021
@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Jun 7, 2021

The latest set of feedback has been addressed.

@atscott
Copy link
Contributor

@atscott atscott commented Jun 11, 2021

@@ -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', () => {

This comment has been minimized.

@alxhub

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 comment has been minimized.

@crisbeto

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.

This comment has been minimized.

@alxhub

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.

@alxhub
Copy link
Contributor

@alxhub alxhub commented Jun 11, 2021

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.

@alxhub
Copy link
Contributor

@alxhub alxhub commented Jun 11, 2021

(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)

@crisbeto crisbeto force-pushed the crisbeto:10277/property-shorthand-declaration branch from 97187a9 to cd917ce Jun 12, 2021
@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Jun 12, 2021

I've addressed the latest set of feedback @alxhub.

@alxhub alxhub force-pushed the crisbeto:10277/property-shorthand-declaration branch from cd917ce to 97e8940 Jun 18, 2021
@google-cla
Copy link

@google-cla google-cla bot commented Jun 18, 2021

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Jun 18, 2021
@alxhub alxhub added cla: yes and removed cla: no labels Jun 18, 2021
@alxhub
alxhub approved these changes Jun 18, 2021
Copy link
Contributor

@alxhub alxhub left a comment

(I've pushed an update that splits out the extra tests for keyed reads into a separate commit, to keep the history clean)

crisbeto added 2 commits Jun 19, 2021
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.
@crisbeto crisbeto force-pushed the crisbeto:10277/property-shorthand-declaration branch from 97e8940 to 9847d6b Jun 19, 2021
@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Jun 21, 2021

@AndrewKushnir suggests running a TGP, I will do so today.

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Jun 21, 2021

This CL is on the 1pm TAP train, should have results this afternoon.

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented Jun 21, 2021

TGP is green.

@dylhunn dylhunn closed this in 699a8b4 Jun 21, 2021
dylhunn added a commit that referenced this pull request Jun 21, 2021
…mplates (#42421)

Adds support for shorthand property declarations inside Angular templates. E.g. doing `{foo, bar}` instead of `{foo: foo, bar: bar}`.

Fixes #10277.

PR Close #42421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment