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.

Sign up for GitHub

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

Jump to bottom

Structural directive autocompletion #40032

Closed
wants to merge 8 commits into from
Closed

Structural directive autocompletion #40032

wants to merge 8 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Dec 8, 2020

No description provided.

@google-cla google-cla bot added the cla: yes label Dec 8, 2020
@mhevery mhevery added the area: compiler Issues related to `ngc`, Angular's template compiler label Dec 9, 2020
@ngbot ngbot bot added this to the Backlog milestone Dec 9, 2020
@alxhub alxhub force-pushed the ngtsc/ls/autocomplete-9 branch 2 times, most recently from 40f6b6f to b459787 Compare December 11, 2020 22:04
@alxhub alxhub marked this pull request as ready for review December 11, 2020 23:36
@pullapprove pullapprove bot added area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) area: language-service Issues related to Angular's VS Code language service comp: ngcc area: security Issues related to built-in security features, such as HTML sanitation labels Dec 11, 2020
zarend
zarend approved these changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@zarend zarend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

A few variable names could be more meaningful .For example isDomProperty gives much more context to the reader than isProperty.

@alxhub
refactor(language-service): add context to template target system
fea0ea9 
This commit extends the template targeting system, which determines the node
being referenced given a template position, to return additional context if
needed about the particular aspect of the node to which the position refers.
For example, a position pointing to an element node may be pointing either
to its tag name or to somewhere in the node body. This is the difference
between `<div|>` and `<div foo | bar>`.
@alxhub
feat(language-service): autocompletion of element tags
59415c8 
This commit expands the autocompletion capabilities of the language service
to include element tag names. It presents both DOM elements from the Angular
DOM schema as well as any components (or directives with element selectors)
that are in scope within the template as options for completion.
@alxhub
refactor(compiler-cli): make TypeCheckingScopeRegistry a general ut…
75a9037 
…ility

The `annotations` package in the compiler previously contained a registry
which tracks NgModule scopes for template type-checking, including unifying
all type-checking metadata across class inheritance lines.

This commit generalizes this utility and prepares it for use in the
`TemplateTypeChecker` as well, to back APIs used by the language service.
@alxhub
refactor(compiler-cli): introduce APIs to support directive autocompl…
535d52b 
…etion

This commit adds two new APIs to the `TemplateTypeChecker`:
`getPotentialDomBindings` and `getDirectiveMetadata`. Together, these will
support the Language Service in performing autocompletion of directive
inputs/outputs.
@alxhub
feat(language-service): complete attributes on elements
8f81140 
This commit adds attribute completion to the Language Service. It completes
from 3 sources:

1. inputs/outputs of directives currently present on the element
2. inputs/outputs/attributes of directives in scope for the element, that
   would become present if the input/output/attribute was added
3. DOM properties and attributes

We distinguish between completion of a property binding (`[foo|]`) and a
completion in an attribute context (`foo|`). For the latter, bindings to
the attribute are offered, as well as a property binding which adds the
square bracket notation.

To determine hypothetical matches (directives which would become present if
a binding is added), directives in scope are scanned and matched against a
hypothetical version of the element which has the attribute.
@alxhub
feat(language-service): autocomplete pipe binding expressions
a8bc91c 
This commit adds autocompletion for pipe expressions, built on existing APIs
for checking which pipes are in scope.
@alxhub
refactor(compiler-cli): identify structural directives
468fa4b 
This commit introduces an `isStructural` flag on directive metadata, which
is `true` if the directive injects `TemplateRef` (and thus is at least
theoretically usable as a structural directive). The flag is not used for
anything currently, but will be utilized by the Language Service to offer
better autocompletion results for structural directives.
@alxhub
feat(language-service): completions for structural directives
4243377 
This comit adds support for autocompletion of attributes that create
structural directives. Such completions differ from those of normal
attributes, as the structural directive syntax creates a synthetic
<ng-template> node which has different attributes from the main element.
@alxhub alxhub added the target: patch This PR is targeted for the next patch release label Dec 14, 2020
jelbourn
jelbourn approved these changes Dec 14, 2020
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the schema file

Reviewed-for: fw-security

packages/compiler/src/schema/dom_element_schema_registry.ts
@@ -240,6 +240,13 @@ const _ATTR_TO_PROP: {[name: string]: string} = {
'tabindex': 'tabIndex',
};

// Invert _ATTR_TO_PROP.
const _PROP_TO_ATTR: {[name: string]: string} =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: could this be Record<string, string>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could. I actually prefer the primitive syntax as it gives the opportunity to name what kind of strings the keys are, which Record<string, string> does not.

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Dec 14, 2020
@alxhub
Copy link
Member Author

alxhub commented Dec 14, 2020

(Note to caretaker/myself: does not require dev-infra approval as only BUILD.bazel files are changed. The compiler-cli has an exception for this, but the language-service is in a separate directory and thus not covered, even though it's still compiler code)

@alxhub alxhub added PullApprove: disable target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Dec 14, 2020
@alxhub alxhub closed this in ccaf48d Dec 14, 2020
alxhub added a commit that referenced this pull request Dec 14, 2020
@alxhub
feat(language-service): autocompletion of element tags (#40032)
e42250f 
This commit expands the autocompletion capabilities of the language service
to include element tag names. It presents both DOM elements from the Angular
DOM schema as well as any components (or directives with element selectors)
that are in scope within the template as options for completion.

PR Close #40032
alxhub added a commit that referenced this pull request Dec 14, 2020
@alxhub
refactor(compiler-cli): make TypeCheckingScopeRegistry a general ut…
a543e69 
…ility (#40032)

The `annotations` package in the compiler previously contained a registry
which tracks NgModule scopes for template type-checking, including unifying
all type-checking metadata across class inheritance lines.

This commit generalizes this utility and prepares it for use in the
`TemplateTypeChecker` as well, to back APIs used by the language service.

PR Close #40032
alxhub added a commit that referenced this pull request Dec 14, 2020
@alxhub
refactor(compiler-cli): introduce APIs to support directive autocompl…
c0ab43f 
…etion (#40032)

This commit adds two new APIs to the `TemplateTypeChecker`:
`getPotentialDomBindings` and `getDirectiveMetadata`. Together, these will
support the Language Service in performing autocompletion of directive
inputs/outputs.

PR Close #40032
alxhub added a commit that referenced this pull request Dec 14, 2020
@alxhub
feat(language-service): complete attributes on elements (#40032)
66378ed 
This commit adds attribute completion to the Language Service. It completes
from 3 sources:

1. inputs/outputs of directives currently present on the element
2. inputs/outputs/attributes of directives in scope for the element, that
   would become present if the input/output/attribute was added
3. DOM properties and attributes

We distinguish between completion of a property binding (`[foo|]`) and a
completion in an attribute context (`foo|`). For the latter, bindings to
the attribute are offered, as well as a property binding which adds the
square bracket notation.

To determine hypothetical matches (directives which would become present if
a binding is added), directives in scope are scanned and matched against a
hypothetical version of the element which has the attribute.

PR Close #40032
alxhub added a commit that referenced this pull request Dec 14, 2020
@alxhub
feat(language-service): autocomplete pipe binding expressions (#40032)
cbb6eae 
This commit adds autocompletion for pipe expressions, built on existing APIs
for checking which pipes are in scope.

PR Close #40032
alxhub added a commit that referenced this pull request Dec 14, 2020
@alxhub
refactor(compiler-cli): identify structural directives (#40032)
c55bf4a 
This commit introduces an `isStructural` flag on directive metadata, which
is `true` if the directive injects `TemplateRef` (and thus is at least
theoretically usable as a structural directive). The flag is not used for
anything currently, but will be utilized by the Language Service to offer
better autocompletion results for structural directives.

PR Close #40032
alxhub added a commit that referenced this pull request Dec 14, 2020
@alxhub
feat(language-service): completions for structural directives (#40032)
2a74431 
This comit adds support for autocompletion of attributes that create
structural directives. Such completions differ from those of normal
attributes, as the structural directive syntax creates a synthetic
<ng-template> node which has different attributes from the main element.

PR Close #40032
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott atscott approved these changes

@jelbourn jelbourn jelbourn approved these changes

@zarend zarend zarend approved these changes

@IgorMinar IgorMinar Awaiting requested review from IgorMinar

@mhevery mhevery Awaiting requested review from mhevery

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: dev-infra Issues related to Angular's own dev infra (build, test, CI, releasing) area: language-service Issues related to Angular's VS Code language service area: security Issues related to built-in security features, such as HTML sanitation cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note PullApprove: disable target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alxhub @atscott @jelbourn @zarend @mhevery