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(ivy): use the schema registry to check DOM bindings #32171

Closed
wants to merge 1 commit into from

Conversation

@alxhub
Copy link
Contributor

commented Aug 16, 2019

Previously, ngtsc attempted to use the .d.ts schema for HTML elements to
check bindings to DOM properties. However, the TypeScript lib.dom.d.ts
schema does not perfectly align with the Angular DomElementSchemaRegistry,
and these inconsistencies would cause issues in apps. There is also the
concern of supporting both CUSTOM_ELEMENTS_SCHEMA and NO_ERRORS_SCHEMA which
would have been very difficult to do in the existing system.

With this commit, the DomElementSchemaRegistry is employed in ngtsc to check
bindings to the DOM. Previous work on producing template diagnostics is used
to support generation of this different kind of error with the same high
quality of error message.


Reviewers: only the last commit is important.

@@ -72,9 +77,19 @@ export interface TypeCheckingConfig {
* checked, but not the assignment of the resulting type to the `input` property of whichever
* directive or component is receiving the binding. If set to `true`, both sides of the assignment
* are checked.
*
* This flag only affects bindings to components/directives. Bindings to the DOM are checked if
* `checkTypeOfDomBindings` is set.
*/
checkTypeOfBindings: boolean;

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

How about renaming this option to checkTypeOfInputBindings?

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

👍


schemas: SchemaMetadata[], sourceMapping: TemplateSourceMapping,
file: ParseSourceFile): void {
const id = this.sourceManager.allocateId(sourceMapping, file);

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

How about

Suggested change
const id = this.sourceManager.allocateId(sourceMapping, file);
const id = this.sourceManager.captureSource(sourceMapping, file);

or similar as it's not only allocating an id, but actually storing the source mapping data for later use.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

👍

* attached to a TCB's function declaration as leading trivia. This enables translation of
* diagnostics produced for TCB code to their source location in the template.
*/
private templateSources = new Map<string, TemplateSource>();

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

Could you please move TemplateSource to below TcbSourceManager, as it's the sole owner of that type. Perhaps even better would be to break out this logic into a separate file.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

Extracted to source.ts :) thanks, this is cleaner.

@@ -371,3 +353,34 @@ function splitStringAtPoints(str: string, points: number[]): string[] {
splits.push(str.substring(start));
return splits;
}

class TcbSourceManager implements TcbSourceResolver {

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

I like this refactor, now that it has substantially more usage sites :)

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

Yeah, it feels like this was a missing piece in the design.

*/

import {DomElementSchemaRegistry, SchemaMetadata, TmplAstElement} from '@angular/compiler';
import {ParseSourceSpan} from '@angular/compiler/src/compiler';

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

Move import to the line above.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

Done.

@@ -299,6 +301,38 @@ class TcbDirectiveOp extends TcbOp {
}
}

class TcbSchemaCheckerOp extends TcbOp {

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

Could use a comment, especially how it relates to TcbUnclaimedInputsOp as these ops are queued in pairs.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

👍 done.

@@ -644,6 +680,7 @@ class Scope {
// to add them if needed.
if (node instanceof TmplAstElement) {
this.opQueue.push(new TcbUnclaimedInputsOp(this.tcb, this, node, claimedInputs));
this.opQueue.push(new TcbSchemaCheckerOp(this.tcb, node, true, claimedInputs));

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member
Suggested change
this.opQueue.push(new TcbSchemaCheckerOp(this.tcb, node, true, claimedInputs));
this.opQueue.push(new TcbSchemaCheckerOp(this.tcb, node, /* checkElement */ true, claimedInputs));

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

Even better, I extracted a variable with a long comment attached to it.

@@ -119,6 +120,8 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = {
checkQueries: false,
checkTemplateBodies: true,
checkTypeOfBindings: true,
// Feature is still in development.

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

Include TODO comment?

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

👍

@@ -34,11 +34,6 @@ describe('type check blocks', () => {
expect(tcb(TEMPLATE)).toContain('((ctx).a)[(ctx).b];');
});

it('should translate unclaimed bindings to their property equivalent', () => {

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

Loosing this test is quite unfortunate, as it's an easy feature to regress in. We could pass a full TypeCheckingConfig to tcb to keep verifying this behavior.

In general, there seems to be no remaining tests for checkTypeOfDomBindings IIUC, as it's disabled in all configurations I stumbled upon.

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

👍 reinstated the test in an experimental section.

*/
checkTypeOfBindings: boolean;

/**
* Whether to check the left-hand side type of binding operations to DOM properties.

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 19, 2019

Member

The invocation of the DOM schema checker is currently not controlled by this flag, but only depending on the NgModule.schemas config. I believe this is the desired behavior as I mentioned in an earlier review, however can we include a note here to mention it explicitly, as any schemas configured will still check DOM bindings according to the schema.

@JoostK

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

As a side note, disabling the DOM bindings in the template type checker for now will "fix" several reported issues around ngBaseDef.

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/schema branch from b352f48 to db8a227 Aug 21, 2019

@JoostK
JoostK approved these changes Aug 21, 2019
Copy link
Member

left a comment

Except for the failing Material tests and a minor comment, LGTM!

expect(tcb(TEMPLATE)).toContain('_t1.htmlFor = ("test");');
});

it('should handle empty bindings', () => {

This comment has been minimized.

Copy link
@JoostK

JoostK Aug 21, 2019

Member

Oops, looks like a bad rebase. Could you restore this test and the one below?

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 21, 2019

Author Contributor

Actually this change was intentional - these tests as written tested the behavior when binding to properties of plain DOM elements. But you're right, their functionality is relevant for binding to directive inputs as well, so I've ported them to test that instead.

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/schema branch from db8a227 to 83de52f Aug 21, 2019

feat(ivy): use the schema registry to check DOM bindings
Previously, ngtsc attempted to use the .d.ts schema for HTML elements to
check bindings to DOM properties. However, the TypeScript lib.dom.d.ts
schema does not perfectly align with the Angular DomElementSchemaRegistry,
and these inconsistencies would cause issues in apps. There is also the
concern of supporting both CUSTOM_ELEMENTS_SCHEMA and NO_ERRORS_SCHEMA which
would have been very difficult to do in the existing system.

With this commit, the DomElementSchemaRegistry is employed in ngtsc to check
bindings to the DOM. Previous work on producing template diagnostics is used
to support generation of this different kind of error with the same high
quality of error message.

@alxhub alxhub force-pushed the alxhub:ngtsc/ttc/schema branch from 83de52f to d8f5014 Aug 21, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@AndrewKushnir this could probably use an Ivy global presubmit prior to landing, just to make sure it doesn't break any apps in g3.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@atscott atscott closed this in 0677cf0 Aug 22, 2019

ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
feat(ivy): use the schema registry to check DOM bindings (angular#32171)
Previously, ngtsc attempted to use the .d.ts schema for HTML elements to
check bindings to DOM properties. However, the TypeScript lib.dom.d.ts
schema does not perfectly align with the Angular DomElementSchemaRegistry,
and these inconsistencies would cause issues in apps. There is also the
concern of supporting both CUSTOM_ELEMENTS_SCHEMA and NO_ERRORS_SCHEMA which
would have been very difficult to do in the existing system.

With this commit, the DomElementSchemaRegistry is employed in ngtsc to check
bindings to the DOM. Previous work on producing template diagnostics is used
to support generation of this different kind of error with the same high
quality of error message.

PR Close angular#32171
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
feat(ivy): use the schema registry to check DOM bindings (angular#32171)
Previously, ngtsc attempted to use the .d.ts schema for HTML elements to
check bindings to DOM properties. However, the TypeScript lib.dom.d.ts
schema does not perfectly align with the Angular DomElementSchemaRegistry,
and these inconsistencies would cause issues in apps. There is also the
concern of supporting both CUSTOM_ELEMENTS_SCHEMA and NO_ERRORS_SCHEMA which
would have been very difficult to do in the existing system.

With this commit, the DomElementSchemaRegistry is employed in ngtsc to check
bindings to the DOM. Previous work on producing template diagnostics is used
to support generation of this different kind of error with the same high
quality of error message.

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