Type checking support for host bindings #60267
Conversation
6832d7d to
7aa4001
Compare
7aa4001 to
0f4911a
Compare
|
Is there a way to support syntax highlighting? From the provided image there is none |
|
Yeah I have it on my TODO to look into syntax highlighting. It's in the vscode extension repo though. |
The vscode extension generally needs more love and care 😁 |
|
@crisbeto i really like to have tailwind support for class names in host binding. Whether it will enabled by this PR? |
There was a problem hiding this comment.
Excited for this feature! 💯
The commit message of the feat commit mentions:
Note that initially the new functionality is hidden behind the
typeCheckHostBindingscompiler flag
Since the option is public, I'd rephrase that to
Note that initially the new functionality is disabled by default and has to be enabled using the
typeCheckHostBindingscompiler flag.
packages/compiler-cli/src/ngtsc/metadata/src/resource_registry.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/annotations/directive/src/handler.ts
Outdated
Show resolved
Hide resolved
| return diag.file!.text.slice(diag.start!, diag.start! + diag.length!); | ||
| } | ||
|
|
||
| runInEachFileSystem(() => { |
There was a problem hiding this comment.
I havent't looked at all tests in detail, but I realised there weren't any cases for window: and document: prefixes and I haven't seen those being handled specially so they're probably incorrectly targeting the host element instead of window/document.
There was a problem hiding this comment.
There's a document:click in the should check host event listeners with a target test and it seems to get inferred as a MouseEvent correctly.
There was a problem hiding this comment.
I suppose that is because the host element for button[foo] also supports the click event, You'd likely see issues for host bindings for properties that are only available on document/window.
There was a problem hiding this comment.
How can you access document or window in the handler expression though? The only way I can think of is $event.target, but even then TS infers it to be EventTarget rather than the node type.
There was a problem hiding this comment.
@Directive({
host: {
'(window:beforeunload)': 'beforeUnload($event)',
},
})
export class MyCmp {
beforeUnload(event: BeforeUnloadEvent) {}
}Since the beforeunload event is mapped to BeforeUnloadEvent in WindowEventHandlersEventMap, it is specific to Window.addEventListener. I expect that we're currently targeting the host element itself, which likely has a different addEventListener that doesn't use WindowEventHandlersEventMap and thus doesn't map to the BeforeUnloadEvent.
There was a problem hiding this comment.
Okay I see what you mean. In that case the event gets inferred as Event, rather than BeforeUnloadEvent. That being said, we don't seem to handle this in the template TCB either. E.g. here's the same setup in a template:
There was a problem hiding this comment.
I'd expect that the target prefix is designed to be used in host listeners, but it could be that they also (unintentionally, maybe?) work in templates?
There was a problem hiding this comment.
It's a bit counter-intuitive, but they work in templates too. E.g. you can do <button (window:scroll)="..."> to listen to the global scroll event "through" some random button.
| // back to the source. E.g. `@HostListener('foo') handleFoo() {}` in source code would look | ||
| // something like `(foo)="handleFoo()"` in the AST. Instead we construct the expressions | ||
| // manually here. Note that we use a dummy span with -1/-1 as offsets, because it isn't | ||
| // used for type checking and constructing it accurately would take some effort. |
There was a problem hiding this comment.
Out of curiosity: since you are saying the source spans here aren't used for type checking, wouldn't it be possible to use a fake expression here with a "synthetic" source file?
There was a problem hiding this comment.
For context, these are the spans within the expression itself, rather than the source file. E.g. the span for 'foo' would be start: 0, end: 6. It's fairly straightforward to count the number of characters for each expression, but I figured it's not worth the hassle considering that it's not used anywhere.
| // back to the source. E.g. `@HostListener('foo') handleFoo() {}` in source code would look | ||
| // something like `(foo)="handleFoo()"` in the AST. Instead we construct the expressions | ||
| // manually here. Note that we use a dummy span with -1/-1 as offsets, because it isn't | ||
| // used for type checking and constructing it accurately would take some effort. |
There was a problem hiding this comment.
For context, these are the spans within the expression itself, rather than the source file. E.g. the span for 'foo' would be start: 0, end: 6. It's fairly straightforward to count the number of characters for each expression, but I figured it's not worth the hassle considering that it's not used anywhere.
| // 4. Constructing some sort of string like `<host ${name.getText()}=${initializer.getText()}/>`, | ||
| // passing it through the HTML parser and extracting the first attribute from it - wasn't explored | ||
| // much, but likely has the same issues as approach #3. |
There was a problem hiding this comment.
Ah interesting. When I was looking for a way to handle this, I was comparing with templates and noticed that it was dealing with it correctly. Too bad escapedString is in the ML parser, rather than the expression parser. In theory we could go with approach #4 so we can take advantage of escapedString, but it'll also come with a bit more overhead.
| return diag.file!.text.slice(diag.start!, diag.start! + diag.length!); | ||
| } | ||
|
|
||
| runInEachFileSystem(() => { |
There was a problem hiding this comment.
There's a document:click in the should check host event listeners with a target test and it seems to get inferred as a MouseEvent correctly.
0f4911a to
845a13c
Compare
packages/compiler-cli/test/ngtsc/host_bindings_type_check_spec.ts
Outdated
Show resolved
Hide resolved
| return diag.file!.text.slice(diag.start!, diag.start! + diag.length!); | ||
| } | ||
|
|
||
| runInEachFileSystem(() => { |
There was a problem hiding this comment.
I suppose that is because the host element for button[foo] also supports the click event, You'd likely see issues for host bindings for properties that are only available on document/window.
36de1f6 to
58fce16
Compare
Following up on angular#30626 (comment), these changes move the button's shared host bindings into the base class and turn some classes that are added manually into host bindings. The previous approach was only necessary with ViewEngine, because it wasn't doing inheritance of host bindings correctly and wasn't merging classes. These are no longer issues in Ivy. Finally, these changes allow us to take advantage of an upcoming framework feature: angular/angular#60267
Following up on angular#30626 (comment), these changes move the button's shared host bindings into the base class and turn some classes that are added manually into host bindings. The previous approach was only necessary with ViewEngine, because it wasn't doing inheritance of host bindings correctly and wasn't merging classes. These are no longer issues in Ivy. Finally, these changes allow us to take advantage of an upcoming framework feature: angular/angular#60267
Makes the necessary changes on the vscode extension side to enable angular/angular#60267, including: * Looking for completions, quick info etc inside the `host` property. * Syntax highlighting for the `host` object literal.
Moves the `typeCheckHostBindings` option into `StrictTemplateOptions` and renames the latter.
58fce16 to
eff1b1f
Compare
Following up on #30626 (comment), these changes move the button's shared host bindings into the base class and turn some classes that are added manually into host bindings. The previous approach was only necessary with ViewEngine, because it wasn't doing inheritance of host bindings correctly and wasn't merging classes. These are no longer issues in Ivy. Finally, these changes allow us to take advantage of an upcoming framework feature: angular/angular#60267
Makes the necessary changes on the vscode extension side to enable angular/angular#60267, including: * Looking for completions, quick info etc inside the `host` property. * Syntax highlighting for the `host` object literal.
Makes the necessary changes on the vscode extension side to enable angular/angular#60267, including: * Looking for completions, quick info etc inside the `host` property. * Syntax highlighting for the `host` object literal.
|
This PR was merged into the repository by commit bf2b025. The changes were merged into the following branches: main |
Updates the `R3TargetBinder` to ingest host element nodes. PR Close #60267
Sets up the infrastructure to track the host bindings of directives for type checking purposes. PR Close #60267
Adds the logic that creates a `HostElement` AST node based on the host bindings of a directive. PR Close #60267
Historically Angular's type checking only extended to templates, however host bindings can contain expressions as well which can have type checking issues of their own. These changes expand the type checking infrastructure to cover the `host` object literal, `@HostBinding` decorators and `@HostListener` with full language service support coming in future commits. Note that initially the new functionality is disabled by default and has to be enabled using the `typeCheckHostBindings` compiler flag. PR Close #60267
Moves the `typeCheckHostBindings` option into `StrictTemplateOptions` and renames the latter. PR Close #60267
The `TemplateLiteralElementExpr` has some logic where it tries to estimate the `rawText` if one isn't provided by looking at the node's source span. The problem with this approach is that we have some long-standing issues with our expression AST parser (see angular#60267 (comment)) where it might not produce accurate spans if escape sequences are involved. This in turn can lead to unrecoverable errors, because TypeScript will throw an error if the raw string doesn't match the cooked one when constructing a TypeScript AST node. These changes remove the logic that depends on the source span and relies purely on the secondary fallback that inserts escaped characters manually. It's also worth noting that the `rawText` doesn't seem to matter much at this point, because the main usage of it is when downlevelling template literals to ES5 which we no longer support. Fixes angular#60528.
The `TemplateLiteralElementExpr` has some logic where it tries to estimate the `rawText` if one isn't provided by looking at the node's source span. The problem with this approach is that we have some long-standing issues with our expression AST parser (see #60267 (comment)) where it might not produce accurate spans if escape sequences are involved. This in turn can lead to unrecoverable errors, because TypeScript will throw an error if the raw string doesn't match the cooked one when constructing a TypeScript AST node. These changes remove the logic that depends on the source span and relies purely on the secondary fallback that inserts escaped characters manually. It's also worth noting that the `rawText` doesn't seem to matter much at this point, because the main usage of it is when downlevelling template literals to ES5 which we no longer support. Fixes #60528. PR Close #60529
) The `TemplateLiteralElementExpr` has some logic where it tries to estimate the `rawText` if one isn't provided by looking at the node's source span. The problem with this approach is that we have some long-standing issues with our expression AST parser (see angular#60267 (comment)) where it might not produce accurate spans if escape sequences are involved. This in turn can lead to unrecoverable errors, because TypeScript will throw an error if the raw string doesn't match the cooked one when constructing a TypeScript AST node. These changes remove the logic that depends on the source span and relies purely on the secondary fallback that inserts escaped characters manually. It's also worth noting that the `rawText` doesn't seem to matter much at this point, because the main usage of it is when downlevelling template literals to ES5 which we no longer support. Fixes angular#60528. PR Close angular#60529
…60753) The `TemplateLiteralElementExpr` has some logic where it tries to estimate the `rawText` if one isn't provided by looking at the node's source span. The problem with this approach is that we have some long-standing issues with our expression AST parser (see #60267 (comment)) where it might not produce accurate spans if escape sequences are involved. This in turn can lead to unrecoverable errors, because TypeScript will throw an error if the raw string doesn't match the cooked one when constructing a TypeScript AST node. These changes remove the logic that depends on the source span and relies purely on the secondary fallback that inserts escaped characters manually. It's also worth noting that the `rawText` doesn't seem to matter much at this point, because the main usage of it is when downlevelling template literals to ES5 which we no longer support. Fixes #60528. PR Close #60529 PR Close #60753
|
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. |
Historically Angular's type checking only extended to templates, however host bindings can contain expressions as well which can have type checking issues of their own (example and example). These changes add the ability to type check the
hostobject literal,@HostBindingand@HostListenerdecorators of@Directiveand@Componentclasses. The new functionality is hidden behind thetypeCheckHostBindingscompiler flag, but we plan to enable it by default in a future version.This also allows us to enable language service features in host bindings like renaming and hover information. Here's demo showing how it's integrated:
Note: the language service integration for this is done, but I'll send it out in a separate PR to make this one easier to review.