feat(ivy): use the schema registry to check DOM bindings by alxhub · Pull Request #32171 · angular/angular

alxhub · 2019-08-16T23:52:50Z

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.

JoostK · 2019-08-19T17:47:20Z

packages/compiler-cli/src/ngtsc/typecheck/src/api.ts

How about renaming this option to checkTypeOfInputBindings?

JoostK · 2019-08-19T17:49:37Z

packages/compiler-cli/src/ngtsc/typecheck/src/context.ts

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.

JoostK · 2019-08-19T17:53:51Z

packages/compiler-cli/src/ngtsc/typecheck/src/context.ts

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.

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

JoostK · 2019-08-19T17:54:32Z

packages/compiler-cli/src/ngtsc/typecheck/src/context.ts

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

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

JoostK · 2019-08-19T17:55:52Z

packages/compiler-cli/src/ngtsc/typecheck/src/dom.ts

Move import to the line above.

JoostK · 2019-08-19T18:07:20Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

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

JoostK · 2019-08-19T18:09:02Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

Suggested change

this.opQueue.push(new TcbSchemaCheckerOp(this.tcb, node, true, claimedInputs));

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

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

JoostK · 2019-08-19T18:11:15Z

packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts

Include TODO comment?

JoostK · 2019-08-19T18:16:58Z

packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts

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.

👍 reinstated the test in an experimental section.

JoostK · 2019-08-19T18:22:18Z

packages/compiler-cli/src/ngtsc/typecheck/src/api.ts

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 · 2019-08-19T18:31:35Z

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

JoostK

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

JoostK · 2019-08-21T19:38:18Z

packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts

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

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.

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 · 2019-08-21T20:36:08Z

@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 · 2019-08-22T00:50:28Z

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

angular-automatic-lock-bot · 2019-09-22T16:02:52Z

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.}

alxhub requested a review from a team August 16, 2019 23:52

googlebot added the cla: yes label Aug 16, 2019

alxhub closed this Aug 16, 2019

alxhub reopened this Aug 16, 2019

alxhub mentioned this pull request Aug 16, 2019

Use the DomElementSchemaRegistry for type checking in legacy mode #30301

Closed

JoostK mentioned this pull request Aug 18, 2019

feat(ivy): convert all ngtsc diagnostics to ts.Diagnostics #31952

Closed

kara added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler comp: ivy labels Aug 19, 2019

ngbot bot modified the milestone: needsTriage Aug 19, 2019

kara added the target: major This PR is targeted for the next major release label Aug 19, 2019

kara requested a review from JoostK August 19, 2019 16:20

JoostK reviewed Aug 19, 2019

View reviewed changes

alxhub force-pushed the ngtsc/ttc/schema branch from b352f48 to db8a227 Compare August 21, 2019 19:09

JoostK approved these changes Aug 21, 2019

View reviewed changes

alxhub force-pushed the ngtsc/ttc/schema branch from db8a227 to 83de52f Compare August 21, 2019 19:47

alxhub force-pushed the ngtsc/ttc/schema branch from 83de52f to d8f5014 Compare August 21, 2019 19:56

alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 21, 2019

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Aug 21, 2019

atscott closed this in 0677cf0 Aug 22, 2019

kara removed the action: presubmit The PR is in need of a google3 presubmit label Aug 22, 2019

thecp mentioned this pull request Aug 23, 2019

Ivy: unknown element test failures with NO_ERRORS_SCHEMA #31670

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 22, 2019

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

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

Conversation

alxhub commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoostK commented Aug 19, 2019

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub commented Aug 21, 2019

Uh oh!

AndrewKushnir commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Sep 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alxhub commented Aug 16, 2019 •

edited

Loading

AndrewKushnir commented Aug 22, 2019 •

edited

Loading