feat(ivy): ngtsc compiles @Component, @Directive, @NgModule by alxhub · Pull Request #24427 · angular/angular

alxhub · 2018-06-11T17:58:08Z

This change supports compilation of components, directives, and modules
within ngtsc. Support is not complete, but is enough to compile and test
//packages/core/test/bundling/todo in full AOT mode. Code size benefits
are not yet achieved as //packages/core itself does not get compiled, and
some decorators (e.g. @input) are not stripped, leading to unwanted code
being retained by the tree-shaker. This will be improved in future commits.

mary-poppins · 2018-06-11T18:11:28Z

You can preview 184cdd3 at https://pr24427-184cdd3.ngbuilds.io/.

JoostK · 2018-06-11T18:48:11Z

packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts

a precompiled

JoostK · 2018-06-11T19:44:14Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

Just a heads up, makeBindingParser is ridiculously slow due to its construction of DomElementSchemaRegistry. I noticed while profiling the Ivy Todo app in JIT mode where all calls to makeBindingParser summed up to ~100ms. Constructing the schema registry only once reduced that to nil.

Hey! Wow, this is good feedback.

makeBindingParser is only temporary - eventually the DomElementSchemaRegistry will (in JIT mode) be something that the user can override, and the BindingParser itself will be rewritten to not depend on Compile*Metadata.

JoostK · 2018-06-11T19:45:02Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

o -> from?

mary-poppins · 2018-06-11T23:02:33Z

You can preview 76d8c04 at https://pr24427-76d8c04.ngbuilds.io/.

mary-poppins · 2018-06-12T02:54:02Z

You can preview 401431f at https://pr24427-401431f.ngbuilds.io/.

mary-poppins · 2018-06-12T05:20:54Z

You can preview 19d8b6e at https://pr24427-19d8b6e.ngbuilds.io/.

trotyl · 2018-06-12T09:44:33Z

packages/compiler-cli/src/ngtsc/annotations/src/component.ts

In current implementation, a component would be given ng-component as its selector when not specified, is this being a breaking change?

angular/packages/compiler/src/metadata_resolver.ts

Lines 348 to 350 in 3ed2d75

if (!selector) {

selector = this._schemaRegistry.getDefaultComponentElementName();

}

(Even I don't think this feature should be depend upon)

This is selector scope registration, which determines what selectors we look for when parsing a template of another component. If a component/directive doesn't have a selector, then it cannot be in the selector scope because it can never match an element in a template.

@alxhub A selector-less component would indeed match <ng-component> in template:

https://stackblitz.com/edit/angular-hxgxhz?file=src%2Fapp%2Fhello.component.ts

....

I have no words.

JoostK · 2018-06-12T18:40:29Z

packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts

Forgot to mention yesterday, I find it somewhat suspicious that in the guard above, the original node mapping is not used. I'm not familiar with what TS considers as "original" node so I can't really say if this is bad or not.

one more occurrence in registerSelector

Yeah, that's a good point. Original nodes are "untransformed" nodes. During the analysis phase (when registerModule and registerSelector) are called, ts.getOriginalNode(node) will always be node, because the original source files are analyzed.

During the emit phase, when TS is running transformers, other transformers (before Angular's) may change the AST, so by the time we go to codegen data for the node, the reference could be different. ts.getOriginalNode walks back from the transformed node to the untransformed one that was seen during our analysis phase.

Here it's not necessary (and is a no-op) as registerModule and registerSelector are always called during the analysis phase, so the nodes we deal with will always be original.

mary-poppins · 2018-06-12T18:46:07Z

You can preview 3632542 at https://pr24427-3632542.ngbuilds.io/.

mary-poppins · 2018-06-12T19:04:59Z

You can preview b26605d at https://pr24427-b26605d.ngbuilds.io/.

vicb · 2018-06-13T18:51:09Z

packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts

vicb · 2018-06-13T18:52:03Z

packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts

vicb · 2018-06-13T19:06:44Z

packages/compiler-cli/src/ngtsc/metadata/test/resolver_spec.ts

nigt: expect(reference).toBeAnInstanceOf(ExternalExpr, 'Expected expression reference to be an external (import) expression')

That doesn't narrow the type of reference :)

mary-poppins · 2018-06-13T22:29:12Z

You can preview e8d5225 at https://pr24427-e8d5225.ngbuilds.io/.

@input

This change supports compilation of components, directives, and modules within ngtsc. Support is not complete, but is enough to compile and test //packages/core/test/bundling/todo in full AOT mode. Code size benefits are not yet achieved as //packages/core itself does not get compiled, and some decorators (e.g. @input) are not stripped, leading to unwanted code being retained by the tree-shaker. This will be improved in future commits.

mary-poppins · 2018-06-14T17:18:51Z

You can preview 38f210a at https://pr24427-38f210a.ngbuilds.io/.

angular-automatic-lock-bot · 2019-09-13T18:21:17Z

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

googlebot added the cla: yes label Jun 11, 2018

JoostK reviewed Jun 11, 2018

View reviewed changes

alxhub force-pushed the ngtsc-todo branch from 184cdd3 to 76d8c04 Compare June 11, 2018 22:48

alxhub force-pushed the ngtsc-todo branch 2 times, most recently from 42f4206 to 401431f Compare June 12, 2018 02:44

alxhub force-pushed the ngtsc-todo branch from 401431f to 19d8b6e Compare June 12, 2018 05:11

trotyl reviewed Jun 12, 2018

View reviewed changes

alxhub force-pushed the ngtsc-todo branch from 19d8b6e to 3632542 Compare June 12, 2018 18:33

alxhub added the target: major This PR is targeted for the next major release label Jun 12, 2018

JoostK reviewed Jun 12, 2018

View reviewed changes

alxhub force-pushed the ngtsc-todo branch from 3632542 to b26605d Compare June 12, 2018 18:45

alxhub force-pushed the ngtsc-todo branch from b26605d to f16331c Compare June 13, 2018 18:27

vicb reviewed Jun 13, 2018

View reviewed changes

vicb approved these changes Jun 13, 2018

View reviewed changes

vicb reviewed Jun 13, 2018

View reviewed changes

alxhub force-pushed the ngtsc-todo branch from f16331c to e8d5225 Compare June 13, 2018 22:18

alxhub force-pushed the ngtsc-todo branch from e8d5225 to 38f210a Compare June 14, 2018 17:01

mhevery approved these changes Jun 14, 2018

View reviewed changes

alxhub added the action: merge The PR is ready for merge by the caretaker label Jun 14, 2018

mhevery closed this in 27bc7dc Jun 14, 2018

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

	if (!selector) {
	selector = this._schemaRegistry.getDefaultComponentElementName();
	}

Conversation

alxhub commented Jun 11, 2018

Uh oh!

mary-poppins commented Jun 11, 2018

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!

mary-poppins commented Jun 11, 2018

Uh oh!

mary-poppins commented Jun 12, 2018

Uh oh!

mary-poppins commented Jun 12, 2018

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!

mary-poppins commented Jun 12, 2018

Uh oh!

mary-poppins commented Jun 12, 2018

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!

mary-poppins commented Jun 13, 2018

Uh oh!

mary-poppins commented Jun 14, 2018

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants