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

fix(language-service): include directives in template AST paths #34747

Open
wants to merge 1 commit into
base: master
from

Conversation

@ayazhafiz
Copy link
Member

ayazhafiz commented Jan 12, 2020

When constructing a path to a template node, the language service
currently expunges all directives ASTs. This commit includes them again,
which simplifies location of symbols where directives are, in fact, the
most specific symbol (like element selectors). This commit will also
help in the implementation of #34564, which relies on determination of
the nearest AST in a path.

One inconsistency this introduces is the location of symbols referencing
components between opening and closing tags in an element. In the past,
querying the definition for both opening and closing tags would result
in a query for the symbol encompassing the entire element.

      v                 cursor
<app-c|omp></app-comp>
~~~~~~~~~~~~~~~~~~~~~~  symbol location
                 v      cursor
<app-comp></app-c|omp>
~~~~~~~~~~~~~~~~~~~~~~  symbol location

Now, this behavior only occurs for closing tags; for opening tags, the
queried symbol is the opening tag precisely because that's where the
component selector applies.

      v                 cursor
<app-c|omp></app-comp>
~~~~~~~~~~~             symbol location
                 v      cursor
<app-comp></app-c|omp>
~~~~~~~~~~~~~~~~~~~~~~  symbol location

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
@ayazhafiz ayazhafiz requested a review from kyliau Jan 12, 2020
@ayazhafiz ayazhafiz requested a review from angular/tools-language-service as a code owner Jan 12, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jan 12, 2020
@googlebot googlebot added the cla: yes label Jan 12, 2020
@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 12, 2020

cc @ivanwonder. This PR touches your impl of reference completions. As there is enough information in referenceAttributeValueCompletions to construct the completions, I think we can get rid of the visitor. What do you think?

@ivanwonder

This comment has been minimized.

Copy link
Contributor

ivanwonder commented Jan 13, 2020

I think the root cause of this problem is structural directives(e.g. ngIf, ngFor). The better way to handle this as described below.

export function findTemplateAstAt(ast: TemplateAst[], position: number): TemplateAstPath {

function findTemplateAstAt(ast: TemplateAst[], position: number, isStructuralDirectives?: boolean)
function isStructuralDirectives(templateString:string, position: number):boolean
check if the templateString[position - n] is '*'.
visitEmbeddedTemplate(ast: EmbeddedTemplateAst, context: any): any {
return this.visitChildren(context, visit => {
// Ignore reference, variable and providers
visit(ast.attrs);
visit(ast.directives);
visit(ast.children);
});
}

If the position is on structural directives, no need to visit the children.
@ayazhafiz What do you think of this solution.

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 13, 2020

For clarity, what is the problem you mention here @ivanwonder? I think we may be thinking of different problems.

@ivanwonder

This comment has been minimized.

Copy link
Contributor

ivanwonder commented Jan 13, 2020

Maybe it's different problems. I talk about my pr #34564 . The structural directives and two-way binding are shorthand form.
For example <test-comp string-model «[(ᐱmodelᐱ)]="title"»></test-comp>, the model and modelChange share the same span. So

visitElement(ast: ElementAst, context: any): any {
return this.visitChildren(context, visit => {
// Ingnore providers
visit(ast.attrs);
visit(ast.inputs);
visit(ast.outputs);
visit(ast.references);
visit(ast.directives);
visit(ast.children);
});
}

If visit(ast.outputs); find a path(modelChange), I think it's safe to return here.
For structural directives <div «*ᐱngIfᐱ="true"»></div>, the directive ngIf and element div share the same span. If the visit(ast.directives); can location the ngIf directive, no need to visit children.
visitEmbeddedTemplate(ast: EmbeddedTemplateAst, context: any): any {
return this.visitChildren(context, visit => {
// Ignore reference, variable and providers
visit(ast.attrs);
visit(ast.directives);
visit(ast.children);
});
}

@ivanwonder

This comment has been minimized.

Copy link
Contributor

ivanwonder commented Jan 13, 2020

a
/ \
b c
If a to b can be confirmed that is the result. we don't need to visit a to c.

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 13, 2020

@ivanwonder I see what you are saying. I don't think it will be necessary to limit what we visit (and I'm worried doing so might run into issues down the road). I will try to take a look today or tomorrow; need to catch up on some stuff in my personal life first.

When constructing a path to a template node, the language service
currently expunges all directives ASTs. This commit includes them again,
which simplifies location of symbols where directives are, in fact, the
most specific symbol (like element selectors). This commit will also
help in the implementation of #34564, which relies on determination of
the nearest AST in a path.

One inconsistency this introduces is the location of symbols referencing
components between opening and closing tags in an element. In the past,
querying the definition for both opening and closing tags would result
in a query for the symbol encompassing the entire element.

```text
      v                 cursor
<app-c|omp></app-comp>
~~~~~~~~~~~~~~~~~~~~~~  symbol location
                 v      cursor
<app-comp></app-c|omp>
~~~~~~~~~~~~~~~~~~~~~~  symbol location
```

Now, this behavior only occurs for closing tags; for opening tags, the
queried symbol is the opening tag precisely because that's where the
component selector applies.

```text
      v                 cursor
<app-c|omp></app-comp>
~~~~~~~~~~~             symbol location
                 v      cursor
<app-comp></app-c|omp>
~~~~~~~~~~~~~~~~~~~~~~  symbol location
```
@ayazhafiz ayazhafiz force-pushed the ayazhafiz:i/directive-in-path branch from 5e53434 to 6da5fc8 Jan 14, 2020
visitDirective(ast: DirectiveAst, context: any): any {
// Ignore the host properties of a directive
const result = this.visitChildren(context, visit => { visit(ast.inputs); });
// We never care about the diretive itself, just its inputs.
if (path[path.length - 1] === ast) {
path.pop();
}
return result;
}
};
Comment on lines 139 to 144

This comment has been minimized.

Copy link
@ivanwonder

ivanwonder Jan 14, 2020

Contributor

I don't think the code here need to be removed. An ElementAst may contain lots of directives. this will cause lots of meaningless directives in path.
I think we should care about the directive itself only when its input is what we need.

visitDirective(ast: DirectiveAst, context: any): any {
      const length = path.length;
      // Ignore the host properties of a directive
      const result = this.visitChildren(context, visit => { visit(ast.inputs); });
      // We never care about the diretive itself, just its inputs.
      if (path[path.length - 1] === ast) {
        path.pop();
      }
      if (path.length > length && path[path.length - 2] !== ast) {
        path.splice(path.length - 1, 0, ast);
      }
      return result;
    }
@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 14, 2020

@ivanwonder

This comment has been minimized.

Copy link
Contributor

ivanwonder commented Jan 15, 2020

Ok, I will wait for your future PR.
Just give some info I consider, I hope it's useful for you.
<test-comp string-model></test-comp>, the path will output path: (3) [ElementAst, DirectiveAst(string-model), DirectiveAst(test-comp)], the last one is what we need, but it depends on
the parser' order(Maybe this is not a problem).
<test-comp *string-model="let element;"></test-comp>(Oh, find a new bug... check the hover for string-model), I think it's the same with the pre one.

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 15, 2020

@ivanwonder

This comment has been minimized.

Copy link
Contributor

ivanwonder commented Jan 16, 2020

@ayazhafiz sorry for the delay, I test on the master branch and only remove the code

 if (path[path.length - 1] === ast) {
        path.pop();
      }

the test case is <test-comp *«string-model»="let ele;"></test-comp>.
the path is path: (3) [EmbeddedTemplateAst, DirectiveAst, DirectiveAst].
the output is
Expected '(component) AppModule.TestComponent: class' to be '(directive) StringModel: typeof StringModel'.
I think the *string-model is a structural directive. Just like the ngFor, ngIf

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 16, 2020

@ivanwonder

This comment has been minimized.

Copy link
Contributor

ivanwonder commented Jan 16, 2020

I think the parser doesn't care about the templateRef and view container which only is used in js runtime or used to generate runtime code(I think). it just parses the asterisk (*) syntax into a <ng-template>.
Or you can create a new structural directive for the test.
The directive can be attribute directives or structural directive, depend on how you use it.
See the code here.
https://stackblitz.com/edit/angular-kge9x7?file=src%2Fapp%2Funless.directive.ts
correct me if I am wrong.

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 16, 2020

The parser does care about view containers (see

if (hasInlineTemplates) {
// The element as a *-attribute
const templateQueryStartIndex = this.contentQueryStartId;
const templateSelector = createElementCssSelector('ng-template', templateMatchableAttrs);
const {directives} = this._parseDirectives(this.selectorMatcher, templateSelector);
const templateBoundDirectivePropNames = new Set<string>();
const templateDirectiveAsts = this._createDirectiveAsts(
true, elName, directives, templateElementOrDirectiveProps, [], element.sourceSpan !, [],
templateBoundDirectivePropNames);
const templateElementProps: t.BoundElementPropertyAst[] = this._createElementPropertyAsts(
elName, templateElementOrDirectiveProps, templateBoundDirectivePropNames);
this._assertNoComponentsNorElementBindingsOnTemplate(
templateDirectiveAsts, templateElementProps, element.sourceSpan !);
const templateProviderContext = new ProviderElementContext(
this.providerViewContext, parent.providerContext !, parent.isTemplateElement,
templateDirectiveAsts, [], [], true, templateQueryStartIndex, element.sourceSpan !);
templateProviderContext.afterElement();
parsedElement = new t.EmbeddedTemplateAst(
[], [], [], templateElementVars, templateProviderContext.transformedDirectiveAsts,
templateProviderContext.transformProviders,
templateProviderContext.transformedHasViewContainer, templateProviderContext.queryMatches,
[parsedElement], ngContentIndex, element.sourceSpan !);
}
return parsedElement;
}
). But you're right, this is not the root cause of the discrepancy you are seeing. The real cause is that the application of a structural directive is an input. For example, the NgIf directive has the [ngIf] selector and an input of name ngIf. Application of *ngIf="some-expr" on an element wraps the element in a template and passes some-expr to the directive input.

The reason this doesn't work on StringModel is because StringModel's input name is model. Change the selector and input name to be the same, like stringModel, and the definitions show up fine.

Note that the issue you were noticing does not exist for "real" structural directives. Querying the definition on *ngIf or *appUnless (per your stackblitz link) gives the correct definition.

@ivanwonder

This comment has been minimized.

Copy link
Contributor

ivanwonder commented Jan 16, 2020

The reason this doesn't work on StringModel is because StringModel's input name is model. Change the selector and input name to be the same, like stringModel, and the definitions show up fine.

<test-comp *string-model="let element;"></test-comp> If the some-expr is let element;, you donn't need rename model.

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Jan 16, 2020

Can you elaborate? This doesn't work for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.