feat(ivy): @NgModule -> ngInjectorDef compilation #22458

alxhub · 2018-02-26T17:18:43Z

This adds compilation of @NgModule providers and imports into
ngInjectorDef statements in generated code. All @NgModule annotations
will be compiled and the @NgModule decorators removed from the
resultant js output.

All @Injectables will also be compiled in Ivy mode, and the decorator
removed.

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-02-26T17:26:37Z

You can preview ce81cfd at https://pr22458-ce81cfd.ngbuilds.io/.

mary-poppins · 2018-02-26T17:43:53Z

You can preview 3d53b4f at https://pr22458-3d53b4f.ngbuilds.io/.

mary-poppins · 2018-02-28T20:02:48Z

You can preview 23a7543 at https://pr22458-23a7543.ngbuilds.io/.

mhevery · 2018-02-28T22:26:07Z

packages/bazel/src/ng_module.bzl

+    outputs = COMMON_OUTPUTS,
+)
+
+ivy_ng_module = rule(


Leave comments behind to remove this once https://github.com/angular/angular/pull/22480/files#diff-80f32f4427df4c91c1fe4d9d821d9338R739 is done.

mhevery · 2018-02-28T22:30:43Z

packages/compiler-cli/src/transformers/program.ts

@@ -32,6 +33,13 @@ import {DTS, GENERATED_FILES, StructureIsReused, TS, createMessageDiagnostic, is
 */
 const MAX_FILE_COUNT_FOR_SINGLE_FILE_EMIT = 20;

+const IVY_DECORATORS = [


=> CLASS_LEVEL_DECORATORS or something which does not have ivy

consider stripping all decorators.

mhevery · 2018-02-28T22:39:18Z

packages/core/src/di/injector.ts


 export const SOURCE = '__source';
 const _THROW_IF_NOT_FOUND = new Object();
 export const THROW_IF_NOT_FOUND = _THROW_IF_NOT_FOUND;
+interface HasInjectableDef<T> {


InjectableType<T> extends Type<T> { }

mhevery · 2018-02-28T22:39:57Z

packages/core/src/di/injector.ts


 export const SOURCE = '__source';
 const _THROW_IF_NOT_FOUND = new Object();
 export const THROW_IF_NOT_FOUND = _THROW_IF_NOT_FOUND;
+interface HasInjectableDef<T> {
+  ngInjectableDef: {


pull this out to interface InjectableDef

mhevery · 2018-02-28T22:42:03Z

packages/core/src/di/injector.ts

+type InjectableDefType<T> = Type<T>& HasInjectableDef<T>;
+type InjectableDefToken<T> = InjectionToken<T>& HasInjectableDef<T>;
+
+type IvyProvider = InjectableDefType<any>| IvyDeepProvider;


Try not to have code names in types.

mhevery · 2018-02-28T22:46:36Z

packages/core/src/di/r3_injector.ts

+ * found in the LICENSE file at https://angular.io/license
+ */
+
+import {Type} from '../type';


mary-poppins · 2018-03-01T00:07:48Z

You can preview 0507b2b at https://pr22458-0507b2b.ngbuilds.io/.

mary-poppins · 2018-03-01T20:31:16Z

You can preview d85bcf6 at https://pr22458-d85bcf6.ngbuilds.io/.

mary-poppins · 2018-03-01T21:28:03Z

You can preview b10cc11 at https://pr22458-b10cc11.ngbuilds.io/.

mary-poppins · 2018-03-01T22:00:06Z

You can preview bf92046 at https://pr22458-bf92046.ngbuilds.io/.

mary-poppins · 2018-03-01T22:12:52Z

You can preview e3bf36c at https://pr22458-e3bf36c.ngbuilds.io/.

mary-poppins · 2018-03-01T22:27:22Z

You can preview 721bf7c at https://pr22458-721bf7c.ngbuilds.io/.

mary-poppins · 2018-03-01T23:07:46Z

You can preview 3864720 at https://pr22458-3864720.ngbuilds.io/.

mary-poppins · 2018-03-01T23:44:26Z

You can preview 2b32ed9 at https://pr22458-2b32ed9.ngbuilds.io/.

chuckjaz · 2018-03-02T17:28:28Z

packages/compiler-cli/src/transformers/lower_expressions.ts

+          shouldLower(node)) {
+        lowerable = true;
+      }
+      if (isLiteralFieldNamed(node, this.lowerableFieldNames) && shouldLower(node) &&


else. It seems pointless to look at these if we already know lowerable is true.

chuckjaz · 2018-03-02T17:33:39Z

packages/compiler-cli/src/transformers/lower_expressions.ts

+      return lowerable;
+    };
+
+    hasLowerableParent = (node: ts.Node | undefined): boolean => {


This logic seems backwards to me. Consider, instead, taking !hasLowerableParent(node) out of isLowerable and call it shouldBeLowered instead. The isLowerable is simply isShouldBeLowered && !alreadyLowered(node) where !alreadyLowered returns true if it is in a tree that is being lowered (that is it has a parent nod that is being lowered). Mutually recursive calls make my head hurt.

chuckjaz · 2018-03-02T17:34:56Z

packages/compiler-cli/src/transformers/program.ts

@@ -32,6 +33,14 @@ import {DTS, GENERATED_FILES, StructureIsReused, TS, createMessageDiagnostic, is
 */
 const MAX_FILE_COUNT_FOR_SINGLE_FILE_EMIT = 20;

+// TODO(alxhub): develop a way to track the origin of metadata annotations and make this robust.


Why can't we do this before this commit?

chuckjaz · 2018-03-02T17:36:14Z

packages/compiler-cli/src/transformers/program.ts

@@ -32,6 +33,14 @@ import {DTS, GENERATED_FILES, StructureIsReused, TS, createMessageDiagnostic, is
 */
 const MAX_FILE_COUNT_FOR_SINGLE_FILE_EMIT = 20;

+// TODO(alxhub): develop a way to track the origin of metadata annotations and make this robust.
+const R3_REIFIED_DECORATORS = [
+  'Component',


These should be StaticSymbols and they are already known by the StaticReflector you just need to ask for them.

chuckjaz · 2018-03-02T17:37:52Z

packages/compiler-cli/src/transformers/program.ts

@@ -99,7 +108,12 @@ class AngularCompilerProgram implements Program {
        this.host = bundleHost;
      }
    }
-    this.loweringMetadataTransform = new LowerMetadataTransform();
+
+    const lowerFields = ['useValue', 'useFactory', 'data'];


I would prefer two constants LOWER_FIELDS and IVY_LOWER_FIELDS and the if selects which to use (or use a ternary operator).

chuckjaz · 2018-03-02T17:38:20Z

packages/compiler-cli/src/transformers/program.ts

@@ -235,7 +249,8 @@ class AngularCompilerProgram implements Program {
        0) {
      return {emitSkipped: true, diagnostics: [], emittedFiles: []};
    }
-    const modules = this.compiler.emitAllPartialModules(this.analyzedModules);
+    const modules =
+        this.compiler.emitAllPartialModules(this.analyzedModules, this._analyzedInjectables !);


Please include a comment explaining why the ! is safe.

chuckjaz · 2018-03-02T17:52:12Z

packages/compiler/src/render3/r3_module_compiler.ts

+};
+type MapLiteral = MapEntry[];
+
+function mapEntry(key: string, value: o.Expression): MapEntry {


We should put this in a more generate location and then refactor the other code to use it.

chuckjaz · 2018-03-02T17:52:20Z

packages/compiler/src/render3/r3_module_compiler.ts

+}
+
+function toMapLiteral(obj: {[key: string]: o.Expression}): o.Expression {
+  return o.literalMap(Object.keys(obj).map(key => ({


chuckjaz · 2018-03-02T17:52:56Z

packages/compiler/src/render3/r3_module_compiler.ts

+  } else if (meta == null) {
+    return o.literal(meta);
+  } else {
+    throw new Error(`Unsupported or unknown metadata: ${meta}`);


Prefix with "Internal error: "

chuckjaz · 2018-03-02T17:53:02Z

packages/compiler/src/render3/r3_module_compiler.ts

+}
+
+function convertMetaToOutput(meta: any, ctx: OutputContext): o.Expression {
+  if (Array.isArray(meta)) {


Will do in a future PR.

mary-poppins · 2018-03-14T23:05:47Z

You can preview ecfb9fb at https://pr22458-ecfb9fb.ngbuilds.io/.

mhevery

We discussed that inject should work in cases where there is no injector present if providedIn: 'root'. Were you planning on implementing it here, or on next PR?
Could you add a js_expected_symbol_test test (see example in packages/core/test/bundling/hello_world/BUILD.bazel. The test should create an injector using createInjector() and than try to inject something. It should that verify that all other values get tree shaken away.
Also could you clean up packages/core/test/render3/compiler_canonical/pending_api_spec.ts

NOTE: These could be done in follow on PRs.

mhevery · 2018-03-14T23:09:57Z

packages/compiler-cli/src/transformers/program.ts

+const R3_LOWER_FIELDS = [...LOWER_FIELDS, 'providers', 'imports', 'exports'];
+
+const R3_REIFIED_DECORATORS = [
+  'Component',


Should Pipe be included in this list?

Yes, and I can do that.

mhevery · 2018-03-14T23:23:14Z

packages/core/src/di/r3_injector.ts

@@ -0,0 +1,408 @@
+/**


I don't see r3_injector_spec.ts. Where are the tests for this?

Renamed definition_injector_spec.ts to r3_injector_spec.ts

mhevery · 2018-03-14T23:26:34Z

packages/core/test/di/definition_injector_spec.ts

+
+describe('InjectorDef-based createInjector()', () => {
+  class CircularA {
+    static ngInjectableDef = {


why not use defineInjectable API here? (and throughout the file)

mhevery · 2018-03-14T23:27:55Z

packages/core/src/di/r3_injector.ts

+   * Flag indicating this injector provides the APP_ROOT_SCOPE token, and thus counts as the
+   * root scope.
+   */
+  private readonly isRootScope: boolean;


Should probably change the name to isRootInjector????

mhevery · 2018-03-14T23:32:44Z

packages/core/test/di/definition_injector_spec.ts

@@ -0,0 +1,208 @@
+/**


this looks like r3_injector_spec.ts?

mhevery · 2018-03-14T23:33:11Z

packages/core/test/di/definition_injector_spec.ts

+    const instance = injector.get(Service);
+    expect(instance instanceof Service).toBeTruthy();
+    expect(injector.get(Service)).toBe(instance);
+  });


nit: cat we have blank like between it and rest of file.

mhevery · 2018-03-14T23:39:23Z

tools/public_api_guard/core/core.d.ts


 /** @experimental */
-export interface InjectableType<T> extends Type<T> {
+export interface InjectableDefType<T> extends Type<T> {


I think this should be InjectableType (no Def)

mary-poppins · 2018-03-15T16:31:52Z

You can preview fc03f68 at https://pr22458-fc03f68.ngbuilds.io/.

mary-poppins · 2018-03-15T16:48:36Z

You can preview 2b653cc at https://pr22458-2b653cc.ngbuilds.io/.

mary-poppins · 2018-03-15T17:35:06Z

You can preview cbb84c7 at https://pr22458-cbb84c7.ngbuilds.io/.

mary-poppins · 2018-03-15T21:42:28Z

You can preview 285665f at https://pr22458-285665f.ngbuilds.io/.

mary-poppins · 2018-03-16T17:23:47Z

You can preview b44118b at https://pr22458-b44118b.ngbuilds.io/.

mary-poppins · 2018-03-16T17:37:21Z

You can preview 5da234e at https://pr22458-5da234e.ngbuilds.io/.

mary-poppins · 2018-03-16T17:38:40Z

You can preview a96efda at https://pr22458-a96efda.ngbuilds.io/.

@NgModule

This adds compilation of @NgModule providers and imports into ngInjectorDef statements in generated code. All @NgModule annotations will be compiled and the @NgModule decorators removed from the resultant js output. All @Injectables will also be compiled in Ivy mode, and the decorator removed.

mary-poppins · 2018-03-16T18:23:37Z

You can preview 92fa681 at https://pr22458-92fa681.ngbuilds.io/.

@NgModule

This adds compilation of @NgModule providers and imports into ngInjectorDef statements in generated code. All @NgModule annotations will be compiled and the @NgModule decorators removed from the resultant js output. All @Injectables will also be compiled in Ivy mode, and the decorator removed. PR Close angular#22458

angular-automatic-lock-bot · 2019-09-13T10:51:28Z

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 review from mhevery and chuckjaz February 26, 2018 17:18

googlebot added the cla: yes label Feb 26, 2018

alxhub force-pushed the injector-def branch from ce81cfd to 3d53b4f Compare February 26, 2018 17:35

alexeagle added the comp: ivy label Feb 26, 2018

alxhub force-pushed the injector-def branch from 3d53b4f to 23a7543 Compare February 28, 2018 19:26

mhevery reviewed Feb 28, 2018

View reviewed changes

alxhub force-pushed the injector-def branch 2 times, most recently from 1d0a481 to 0507b2b Compare February 28, 2018 23:51

alxhub force-pushed the injector-def branch from 0507b2b to d85bcf6 Compare March 1, 2018 20:23

alxhub force-pushed the injector-def branch from d85bcf6 to b10cc11 Compare March 1, 2018 21:19

alxhub force-pushed the injector-def branch 2 times, most recently from bf92046 to e3bf36c Compare March 1, 2018 21:57

alxhub force-pushed the injector-def branch from e3bf36c to 721bf7c Compare March 1, 2018 22:19

alxhub force-pushed the injector-def branch from 721bf7c to 3864720 Compare March 1, 2018 22:45

alxhub force-pushed the injector-def branch from 3864720 to 2b32ed9 Compare March 1, 2018 23:33

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 Mar 2, 2018

chuckjaz suggested changes Mar 2, 2018

View reviewed changes

chuckjaz approved these changes Mar 14, 2018

View reviewed changes

angular deleted a comment from alxhub Mar 14, 2018

mhevery suggested changes Mar 14, 2018

View reviewed changes

trotyl mentioned this pull request Mar 15, 2018

[WIP] feat(compiler-cli): support RegExp literal in metadata #22283

Closed

3 tasks

alxhub force-pushed the injector-def branch from ecfb9fb to fc03f68 Compare March 15, 2018 16:21

alxhub force-pushed the injector-def branch from fc03f68 to 2b653cc Compare March 15, 2018 16:41

alxhub force-pushed the injector-def branch from 2b653cc to cbb84c7 Compare March 15, 2018 17:27

alxhub force-pushed the injector-def branch from cbb84c7 to 285665f Compare March 15, 2018 21:31

mhevery approved these changes Mar 15, 2018

View reviewed changes

alxhub force-pushed the injector-def branch 2 times, most recently from cc3a4dd to b44118b Compare March 16, 2018 17:16

alxhub force-pushed the injector-def branch 2 times, most recently from 5da234e to a96efda Compare March 16, 2018 17:30

alxhub force-pushed the injector-def branch from a96efda to 92fa681 Compare March 16, 2018 18:15

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

mhevery closed this in 6ef9f22 Mar 16, 2018

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

feat(ivy): @NgModule -> ngInjectorDef compilation #22458

feat(ivy): @NgModule -> ngInjectorDef compilation #22458

Conversation

alxhub commented Feb 26, 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 Feb 26, 2018

mary-poppins commented Feb 26, 2018

mary-poppins commented Feb 28, 2018

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 Mar 1, 2018

mary-poppins commented Mar 1, 2018

mary-poppins commented Mar 1, 2018

mary-poppins commented Mar 1, 2018

mary-poppins commented Mar 1, 2018

mary-poppins commented Mar 1, 2018

mary-poppins commented Mar 1, 2018

mary-poppins commented Mar 1, 2018

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

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 Mar 14, 2018

mhevery left a comment • 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

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

mary-poppins commented Mar 15, 2018

mary-poppins commented Mar 15, 2018

mary-poppins commented Mar 15, 2018

mary-poppins commented Mar 16, 2018

mary-poppins commented Mar 16, 2018

mary-poppins commented Mar 16, 2018

mary-poppins commented Mar 16, 2018

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

mhevery left a comment •

edited