refactor(ivy): first pass at extracting ReflectionHost for abstract reflection #24541

alxhub · 2018-06-15T16:59:35Z

ngtsc needs to reflect over code to property compile it. It performs operations
such as enumerating decorators on a type, reading metadata from constructor
parameters, etc.

Depending on the format (ES5, ES6, etc) of the underlying code, the AST
structures over which this reflection takes place can be very different. For
example, in TS/ES6 code class declarations are ts.ClassDeclaration nodes,
but in ES5 code they've been downleveled to ts.VariableDeclaration nodes that
are initialized to IIFEs that build up the classes being defined.

The ReflectionHost abstraction allows ngtsc to perform these operations without
directly querying the AST. Different implementations of ReflectionHost allow
support for different code formats.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

mary-poppins · 2018-06-15T17:13:12Z

You can preview fa66f9a at https://pr24541-fa66f9a.ngbuilds.io/.

mary-poppins · 2018-06-18T17:10:33Z

You can preview f9f98fa at https://pr24541-f9f98fa.ngbuilds.io/.

mary-poppins · 2018-06-18T17:52:07Z

You can preview 97f14a2 at https://pr24541-97f14a2.ngbuilds.io/.

petebacondarwin · 2018-06-19T07:54:51Z

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

-  args: ts.Expression[];
-}
+export class TypeScriptReflectionHost implements ReflectionHost {
+  constructor(private checker: ts.TypeChecker) {}


please make checker "protected" so that we can inherit from TypeScriptReflectionHost more easily.

Done.

I'm in general nervous about this - it's not in TypeScriptReflectionHost's contract to support ES6 code.

gkalpak · 2018-06-19T10:47:39Z

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

+        if (analysis.analysis !== undefined) {
+          this.analysis.set(node, {
+            adapter,
+            analysis: analysis.analysis, decorator,


Maybe this needs a little more analysis 😛

@gkalpak https://www.youtube.com/watch?v=Vhh_GeBPOhs

Agreed, it could use a little more... ;)

gkalpak · 2018-06-19T10:50:45Z

packages/compiler-cli/src/ngtsc/host/BUILD.bazel

+    ]),
+    module_name = "@angular/compiler-cli/src/ngtsc/host",
+    deps = [
+        "//packages/compiler",


Why this dependency?

I just like slow builds.

Removed ;)

gkalpak · 2018-06-19T10:53:51Z

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

+  private _readSelectorFromCompiledClass(clazz: ts.Declaration): string|null {
+    const def = this.reflector.getMembersOfClass(clazz).find(
+        field =>
+            field.isStatic && (field.name === 'ngComponentDef' || field.name === 'ngDirectiveDef'));


It is not possible for a class to have both decorators, right?
(Probably isn't; just checking 😃)

Correct, it's not.

gkalpak · 2018-06-19T11:00:08Z

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

-  constructor(private checker: ts.TypeChecker, private scopeRegistry: SelectorScopeRegistry) {}
+  constructor(
+      private checker: ts.TypeChecker, private reflector: ReflectionHost,
+      private scopeRegistry: SelectorScopeRegistry, ) {}


Trailing comma (or missing newline) 😱

0️⃣days since clang-format made code less readable

…eflection ngtsc needs to reflect over code to property compile it. It performs operations such as enumerating decorators on a type, reading metadata from constructor parameters, etc. Depending on the format (ES5, ES6, etc) of the underlying code, the AST structures over which this reflection takes place can be very different. For example, in TS/ES6 code `class` declarations are `ts.ClassDeclaration` nodes, but in ES5 code they've been downleveled to `ts.VariableDeclaration` nodes that are initialized to IIFEs that build up the classes being defined. The ReflectionHost abstraction allows ngtsc to perform these operations without directly querying the AST. Different implementations of ReflectionHost allow support for different code formats.

mary-poppins · 2018-06-19T18:24:34Z

You can preview 4583e3c at https://pr24541-4583e3c.ngbuilds.io/.

angular-automatic-lock-bot · 2019-09-13T18:22:43Z

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 15, 2018

alxhub force-pushed the ngtsc-host branch from fa66f9a to f9f98fa Compare June 18, 2018 17:01

alxhub force-pushed the ngtsc-host branch from f9f98fa to 97f14a2 Compare June 18, 2018 17:42

petebacondarwin reviewed Jun 19, 2018

View reviewed changes

gkalpak reviewed Jun 19, 2018

View reviewed changes

gkalpak approved these changes Jun 19, 2018

View reviewed changes

alxhub force-pushed the ngtsc-host branch from 97f14a2 to 4583e3c Compare June 19, 2018 18:12

alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Jun 21, 2018

mhevery approved these changes Jun 21, 2018

View reviewed changes

mhevery closed this in 10da6a4 Jun 21, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ivy): first pass at extracting ReflectionHost for abstract reflection #24541

refactor(ivy): first pass at extracting ReflectionHost for abstract reflection #24541

alxhub commented Jun 15, 2018

mary-poppins commented Jun 15, 2018

mary-poppins commented Jun 18, 2018

mary-poppins commented Jun 18, 2018

petebacondarwin Jun 19, 2018

alxhub Jun 19, 2018 •

edited

gkalpak Jun 19, 2018

JoostK Jun 19, 2018

alxhub Jun 19, 2018

gkalpak Jun 19, 2018

alxhub Jun 19, 2018

gkalpak Jun 19, 2018

alxhub Jun 19, 2018

gkalpak Jun 19, 2018

alxhub Jun 19, 2018

mary-poppins commented Jun 19, 2018

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

refactor(ivy): first pass at extracting ReflectionHost for abstract reflection #24541

refactor(ivy): first pass at extracting ReflectionHost for abstract reflection #24541

Conversation

alxhub commented Jun 15, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

mary-poppins commented Jun 15, 2018

mary-poppins commented Jun 18, 2018

mary-poppins commented Jun 18, 2018

Choose a reason for hiding this comment

alxhub Jun 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Jun 19, 2018

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

alxhub Jun 19, 2018 •

edited