feat(ivy): support generation of flags for directive injection #23345

mhevery · 2018-04-12T19:30:53Z

This change changes:

compiler uses directiveInject instead of inject for Directives
unifies the flags in di as well as render3
changes the signature of directiveInject to match inject In prep for inject and directiveInject need bridging when used with Component.provides #23330
compiler now generates flags for injection.

Compiler portion of #23342
Prep for #23330

feat(ivy): support injection even if no injector present

Remove default injection value from inject / directiveInject since
it is not possible to set using annotations.
Module Injector is stored on LView instead of LInjector data
structure because it can change only at LView level. (More efficient)
Add ngInjectableDef to IterableDiffers so that existing tests can
pass as well as enable IterableDiffers to be injectable without
Injector

kara

Looks good to me, except for the fdescribe

kara · 2018-04-12T19:34:34Z

packages/compiler/test/render3/r3_view_compiler_di_spec.ts

+import {MockDirectory, setup} from '../aot/test_util';
+import {compile, expectEmit} from './mock_compile';
+
+fdescribe('compiler compliance: dependency injection', () => {


Remove fdescribe?

kara · 2018-04-12T19:38:27Z

packages/core/src/render3/di.ts

- * @param token The token associated with the error
- * @returns The error that was created
- */
-function createInjectionError(text: string, token: any) {


Did you mean to remove this? Looks like it's still used in this file.

Working on two things in one PR, reverted

kara

LGTM

mary-poppins · 2018-04-12T19:47:08Z

You can preview 4d6700a at https://pr23345-4d6700a.ngbuilds.io/.

vicb · 2018-04-12T20:04:58Z

packages/compiler/src/render3/r3_view_compiler.ts

            token.identifier != null ? outputCtx.importExpr(tokenRef) : o.literal(tokenRef);
-        args.push(o.importExpr(R3.inject).callFn([value]));
+        const directiveInjectArgs = [tokenValue];
+        let flags = InjectFlags.Default;


const flags = moveThatLogicFurtherDown(dependency); ?

vicb · 2018-04-12T20:07:58Z

packages/compiler/test/render3/r3_view_compiler_di_spec.ts

+    };
+
+    // The template should look like this (where IDENT is a wild card for an identifier):
+    const template = `


rename template to factory or injectxxxyyy ? (comment, var name and expect)
(I'm also ok with removing this comment)
Use $r3$ instead of IDENT for consistency with other tests ?

mary-poppins · 2018-04-12T20:14:26Z

You can preview 7b8bcef at https://pr23345-7b8bcef.ngbuilds.io/.

mary-poppins · 2018-04-12T20:19:06Z

You can preview 8fd7d95 at https://pr23345-8fd7d95.ngbuilds.io/.

mary-poppins · 2018-04-12T20:43:41Z

You can preview c75e17f at https://pr23345-c75e17f.ngbuilds.io/.

mary-poppins · 2018-04-12T21:00:43Z

You can preview 49cb591 at https://pr23345-49cb591.ngbuilds.io/.

vicb · 2018-04-12T23:14:51Z

packages/core/test/bundling/todo/index.ts

-  // TODO(misko) Fix injection
-  // constructor(todoStore: TodoStore) { this.todoStore = todoStore; }
-  constructor() { this.todoStore = new TodoStore(); }
+  constructor(todoStore: TodoStore) { this.todoStore = todoStore; }


constructor(public todoStore: TodoStore) {} should work now

mary-poppins · 2018-04-12T23:17:39Z

You can preview 493ab9b at https://pr23345-493ab9b.ngbuilds.io/.

kara

A few small typos, otherwise looks good to me

kara · 2018-04-13T00:37:07Z

packages/core/test/render3/di_spec.ts

+      }
+
+      class MyComponent {
+        constructor(public mySerivce: MyService) {}


Rename to myService ?

kara · 2018-04-13T00:37:24Z

packages/core/test/render3/di_spec.ts

@@ -397,6 +397,35 @@ describe('di', () => {
      expect(log).toEqual(['DirB', 'DirB', 'DirA (dep: DirB - 2)']);
    });

+    it('should crete instance even when no injector present', () => {


crete -> create

kara

LGTM

mary-poppins · 2018-04-13T00:59:25Z

You can preview 091c9e1 at https://pr23345-091c9e1.ngbuilds.io/.

mary-poppins · 2018-04-13T01:01:04Z

You can preview 019b4c1 at https://pr23345-019b4c1.ngbuilds.io/.

mary-poppins · 2018-04-13T01:05:54Z

You can preview e2a3a2f at https://pr23345-e2a3a2f.ngbuilds.io/.

vicb · 2018-04-13T02:06:03Z

packages/core/src/di/defs.ts

@@ -107,6 +121,7 @@ export function defineInjectable<T>(opts: {
  return {
    providedIn: (opts.providedIn as InjectorType<any>| 'root' | null | undefined) || null,


vicb · 2018-04-13T02:12:13Z

packages/core/src/di/injector.ts

+  } else if (_currentInjector === null) {
+    const injectableDef: InjectableDef<T> = (token as any).ngInjectableDef;
+    if (injectableDef && injectableDef.providedIn == 'root') {
+      return injectableDef.value === undefined ? injectableDef.value = injectableDef.factory() :


do we want to be able to provide undefined by using a special value (ie {}) instead ?

I was thinking about it, and decided that it is not worth the complexity. We can always added later.

vicb · 2018-04-13T02:15:47Z

packages/core/src/render3/interfaces/view.ts

@@ -189,6 +190,11 @@ export interface LView {
   * Queries active for this view - nodes from a view are reported to those queries
   */
  queries: LQueries|null;
+
+  /**
+   * Injector to be used (if any).


useful comment ?

updated comment.

trotyl · 2018-04-13T03:35:56Z

packages/core/src/change_detection/differs/iterable_differs.ts

@@ -135,6 +137,11 @@ export interface IterableDifferFactory {
 *
 */
 export class IterableDiffers {
+  static ngInjectableDef: InjectableDef<IterableDiffers> = defineInjectable({


Why would compilation result lives in source code? Shouldn't it using @Injectable() decorator?

Few reasons:
1.) we currently don't compile core with ivy so the decorator does not get lowered.
2.) There is a circular dependency between the compiler and core which makes it hard for the compiler to compile tho core. Rather than solving that, we have opted for hand coding it.

This comment should probably be in the source code

mary-poppins · 2018-04-13T04:20:13Z

You can preview 27674e3 at https://pr23345-27674e3.ngbuilds.io/.

mary-poppins · 2018-04-13T05:14:24Z

You can preview 685546e at https://pr23345-685546e.ngbuilds.io/.

IgorMinar · 2018-04-13T07:29:00Z

this PR changes the API surface. @mhevery let's discuss tomorrow before merging this.

mhevery · 2018-04-13T19:52:30Z

Discussed with @IgorMinar and we agreed to merge.

IgorMinar · 2018-04-13T20:19:48Z

we agreed because the api is wrong and needs to be corrected before the release.

This change changes: - compiler uses `directiveInject` instead of `inject` for `Directive`s - unifies the flags in `di` as well as `render3` - changes the signature of `directiveInject` to match `inject` In prep for angular#23330 - compiler now generates flags for injection. Compiler portion of angular#23342 Prep for angular#23330

- Remove default injection value from `inject` / `directiveInject` since it is not possible to set using annotations. - Module `Injector` is stored on `LView` instead of `LInjector` data structure because it can change only at `LView` level. (More efficient) - Add `ngInjectableDef` to `IterableDiffers` so that existing tests can pass as well as enable `IterableDiffers` to be injectable without `Injector`

mary-poppins · 2018-04-13T20:58:18Z

You can preview 7cf605d at https://pr23345-7cf605d.ngbuilds.io/.

- Remove default injection value from `inject` / `directiveInject` since it is not possible to set using annotations. - Module `Injector` is stored on `LView` instead of `LInjector` data structure because it can change only at `LView` level. (More efficient) - Add `ngInjectableDef` to `IterableDiffers` so that existing tests can pass as well as enable `IterableDiffers` to be injectable without `Injector` PR Close #23345

PR Close #23345

) PR Close #23345

alfaproject · 2018-04-14T07:36:46Z

tools/public_api_guard/core/core.d.ts

@@ -376,8 +376,10 @@ export interface InjectDecorator {

 export declare const enum InjectFlags {
    Default = 0,
-    SkipSelf = 1,


Why change the order? It's a bitmask anyway o:

angular-automatic-lock-bot · 2019-09-13T21:22:00Z

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 Apr 12, 2018

mhevery requested a review from kara April 12, 2018 19:30

kara reviewed Apr 12, 2018

View reviewed changes

mhevery force-pushed the injection_fix branch 3 times, most recently from 520e435 to 644e714 Compare April 12, 2018 19:41

kara approved these changes Apr 12, 2018

View reviewed changes

mhevery force-pushed the injection_fix branch 2 times, most recently from 7b8bcef to 8fd7d95 Compare April 12, 2018 20:05

vicb reviewed Apr 12, 2018

View reviewed changes

jasonaden added the comp: ivy label Apr 12, 2018

mhevery force-pushed the injection_fix branch from 8fd7d95 to c75e17f Compare April 12, 2018 20:32

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

mhevery force-pushed the injection_fix branch from c75e17f to 49cb591 Compare April 12, 2018 20:50

vicb reviewed Apr 12, 2018

View reviewed changes

mhevery force-pushed the injection_fix branch from 493ab9b to 091c9e1 Compare April 13, 2018 00:50

kara approved these changes Apr 13, 2018

View reviewed changes

mhevery force-pushed the injection_fix branch 2 times, most recently from 019b4c1 to e2a3a2f Compare April 13, 2018 00:57

kara approved these changes Apr 13, 2018

View reviewed changes

vicb reviewed Apr 13, 2018

View reviewed changes

trotyl reviewed Apr 13, 2018

View reviewed changes

mhevery added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Apr 13, 2018

mhevery force-pushed the injection_fix branch from 27674e3 to 685546e Compare April 13, 2018 05:05

IgorMinar added state: blocked and removed action: merge The PR is ready for merge by the caretaker labels Apr 13, 2018

mhevery added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Apr 13, 2018

mhevery added 4 commits April 13, 2018 13:36

docs(ivy): Clean up incorrect comments

8e0d74b

fix(ivy): Update Todo app to take advantage of optional injector

7cf605d

mhevery force-pushed the injection_fix branch from 685546e to 7cf605d Compare April 13, 2018 20:36

IgorMinar closed this in 6f213a7 Apr 13, 2018

IgorMinar pushed a commit that referenced this pull request Apr 13, 2018

docs(ivy): Clean up incorrect comments (#23345)

4384a92

PR Close #23345

IgorMinar pushed a commit that referenced this pull request Apr 13, 2018

fix(ivy): Update Todo app to take advantage of optional injector (#23345

f4017ce

) PR Close #23345

alfaproject reviewed Apr 14, 2018

View reviewed changes

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

		@@ -107,6 +121,7 @@ export function defineInjectable<T>(opts: {
		return {
		providedIn: (opts.providedIn as InjectorType<any>\| 'root' \| null \| undefined) \|\| null,

feat(ivy): support generation of flags for directive injection #23345

feat(ivy): support generation of flags for directive injection #23345

Conversation

mhevery commented Apr 12, 2018 • edited

kara left a 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

kara left a comment

Choose a reason for hiding this comment

mary-poppins commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Apr 12, 2018

mary-poppins commented Apr 12, 2018

mary-poppins commented Apr 12, 2018

mary-poppins commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Apr 12, 2018

kara left a 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

kara left a comment

Choose a reason for hiding this comment

mary-poppins commented Apr 13, 2018

mary-poppins commented Apr 13, 2018

mary-poppins commented Apr 13, 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

mary-poppins commented Apr 13, 2018

mary-poppins commented Apr 13, 2018

IgorMinar commented Apr 13, 2018

mhevery commented Apr 13, 2018

IgorMinar commented Apr 13, 2018

mary-poppins commented Apr 13, 2018

Choose a reason for hiding this comment

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

mhevery commented Apr 12, 2018 •

edited

vicb Apr 12, 2018 •

edited