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): test case #1

Open
wants to merge 2 commits into
base: i/directive-in-path
Choose a base branch
from

Conversation

ivanwonder
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

ayazhafiz and others added 2 commits January 13, 2020 19:10
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 angular#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
```
@ivanwonder
Copy link
Author

this is the test case I use which will report an error. @ayazhafiz

@ayazhafiz
Copy link
Owner

@ivanwonder can you add an input to UnlessDirective of name appUnless? That will fix the error. The input is required to the structural directive.

@ivanwonder
Copy link
Author

If the directive doesn't need the input and it output something itself, the user only needs to care about the template context.
Like the matCellDef from the angular component.
https://stackblitz.com/angular/xbbbogjgmly?file=src%2Fapp%2Ftable-basic-example.html

@ayazhafiz
Copy link
Owner

You're right, my bad. Okay, I don't think that the language service has a concept of a structural directive except for in an attribute. I know almost nothing about the validation of a structural directive outside from what is in the public documentation. It seems that at least they require a TemplateRef and a view provider (well, at least a provider - maybe the type doesn't matter, i don't know).

In any case, the parser does not account for template bindings as an attribute (https://github.com/angular/angular/blob/e0ad9ecda0b8a541b405d2ab35335b90ceb21fd1/packages/compiler/src/template_parser/template_parser.ts#L315-L319), so the language service never knows that attribute exists there. We can only sometimes know via a descendant, like an input or output. If I recall correctly, this is because the parser is designed to apply a directive to a whole element, and thus the span of that directive's application is the whole opening tag of an element -- so the embedded template AST probably shows up, but in a case like <div *appUnless="expr"> UnlessDirective sits on the span of the whole element (I think). Maybe this could be fixed by backpedaling on the embedded template AST; not sure what that would look like though. My guess is that this will be a non-issue when the language service migrates to Ivy and R3 AST, which I believe expresses bound template attributes a little more "close to the source" (see https://github.com/angular/angular/blob/e0ad9ecda0b8a541b405d2ab35335b90ceb21fd1/packages/compiler-cli/src/ngtsc/indexer/src/template.ts#L185-L206). But it has been a while since I have seen this, so I may be wrong. Will investigate more later.

Curious to hear your thoughts.

@ivanwonder
Copy link
Author

You're right, my bad. Okay, I don't think that the language service has a concept of a structural directive except for in an attribute. I know almost nothing about the validation of a structural directive outside from what is in the public documentation. It seems that at least they require a TemplateRef and a view provider (well, at least a provider - maybe the type doesn't matter, i don't know).

Yes, it's difficult for language service to distinguish between them. The user can inject the TemplateRef and ViewContainer, but they can choose not to use them. For the *matCellDef, I think it ignores the ViewContainer.

In any case, the parser does not account for template bindings as an attribute (https://github.com/angular/angular/blob/e0ad9ecda0b8a541b405d2ab35335b90ceb21fd1/packages/compiler/src/template_parser/template_parser.ts#L315-L319), so the language service never knows that attribute exists there. We can only sometimes know via a descendant, like an input or output. If I recall correctly, this is because the parser is designed to apply a directive to a whole element, and thus the span of that directive's application is the whole opening tag of an element -- so the embedded template AST probably shows up, but in a case like <div *appUnless="expr"> UnlessDirective sits on the span of the whole element (I think). Maybe this could be fixed by backpedaling on the embedded template AST; not sure what that would look like though. My guess is that this will be a non-issue when the language service migrates to Ivy and R3 AST, which I believe expresses bound template attributes a little more "close to the source" (see https://github.com/angular/angular/blob/e0ad9ecda0b8a541b405d2ab35335b90ceb21fd1/packages/compiler-cli/src/ngtsc/indexer/src/template.ts#L185-L206). But it has been a while since I have seen this, so I may be wrong. Will investigate more later.

I agree. and I haven't yet read the Ivy code, but the TmplAstBoundAttribute looks great.

Maybe this could be fixed by backpedaling on the embedded template AST; not sure what that would look like though

If include all DirectiveAst, the visitDirective should check which one is the right. And for the structural directive, the span will be wrong. You should find the HTML attribute for it.

ayazhafiz pushed a commit that referenced this pull request May 14, 2020
…ular#36211)

This optimization builds on a lot of prior work to finally make type-
checking of templates incremental.

Incrementality requires two main components:
- the ability to reuse work from a prior compilation.
- the ability to know when changes in the current program invalidate that
  prior work.

Prior to this commit, on every type-checking pass the compiler would
generate new .ngtypecheck files for each original input file in the program.

1. (Build #1 main program): empty .ngtypecheck files generated for each
   original input file.

2. (Build #1 type-check program): .ngtypecheck contents overridden for those
   which have corresponding components that need type-checked.

3. (Build angular#2 main program): throw away old .ngtypecheck files and generate
   new empty ones.

4. (Build angular#2 type-check program): same as step 2.

With this commit, the `IncrementalDriver` now tracks template type-checking
_metadata_ for each input file. The metadata contains information about
source mappings for generated type-checking code, as well as some
diagnostics which were discovered at type-check analysis time. The actual
type-checking code is stored in the TypeScript AST for type-checking files,
which is now re-used between programs as follows:

1. (Build #1 main program): empty .ngtypecheck files generated for each
   original input file.

2. (Build #1 type-check program): .ngtypecheck contents overridden for those
   which have corresponding components that need type-checked, and the
   metadata registered in the `IncrementalDriver`.

3. (Build angular#2 main program): The `TypeCheckShimGenerator` now reuses _all_
   .ngtypecheck `ts.SourceFile` shims from build #1's type-check program in
   the construction of build angular#2's main program. Some of the contents of
   these files might be stale (if a component's template changed, for
   example), but wholesale reuse here prevents unnecessary changes in the
   contents of the program at this point and makes TypeScript's job a lot
   easier.

4. (Build angular#2 type-check program): For those input files which have not
   "logically changed" (meaning components within are semantically the same
   as they were before), the compiler will re-use the type-check file
   metadata from build #1, and _not_ generate a new .ngtypecheck shim.
   For components which have logically changed or where the previous
   .ngtypecheck contents cannot otherwise be reused, code generation happens
   as before.

PR Close angular#36211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants