-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ivy): support generation of flags for directive injection #23345
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, except for the fdescribe
import {MockDirectory, setup} from '../aot/test_util'; | ||
import {compile, expectEmit} from './mock_compile'; | ||
|
||
fdescribe('compiler compliance: dependency injection', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove fdescribe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
* @param token The token associated with the error | ||
* @returns The error that was created | ||
*/ | ||
function createInjectionError(text: string, token: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to remove this? Looks like it's still used in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on two things in one PR, reverted
520e435
to
644e714
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
You can preview 4d6700a at https://pr23345-4d6700a.ngbuilds.io/. |
7b8bcef
to
8fd7d95
Compare
token.identifier != null ? outputCtx.importExpr(tokenRef) : o.literal(tokenRef); | ||
args.push(o.importExpr(R3.inject).callFn([value])); | ||
const directiveInjectArgs = [tokenValue]; | ||
let flags = InjectFlags.Default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const flags = moveThatLogicFurtherDown(dependency);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted
}; | ||
|
||
// The template should look like this (where IDENT is a wild card for an identifier): | ||
const template = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
You can preview 7b8bcef at https://pr23345-7b8bcef.ngbuilds.io/. |
You can preview 8fd7d95 at https://pr23345-8fd7d95.ngbuilds.io/. |
You can preview c75e17f at https://pr23345-c75e17f.ngbuilds.io/. |
You can preview 49cb591 at https://pr23345-49cb591.ngbuilds.io/. |
// TODO(misko) Fix injection | ||
// constructor(todoStore: TodoStore) { this.todoStore = todoStore; } | ||
constructor() { this.todoStore = new TodoStore(); } | ||
constructor(todoStore: TodoStore) { this.todoStore = todoStore; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constructor(public todoStore: TodoStore) {}
should work now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
You can preview 493ab9b at https://pr23345-493ab9b.ngbuilds.io/. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small typos, otherwise looks good to me
} | ||
|
||
class MyComponent { | ||
constructor(public mySerivce: MyService) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to myService
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -397,6 +397,35 @@ describe('di', () => { | |||
expect(log).toEqual(['DirB', 'DirB', 'DirA (dep: DirB - 2)']); | |||
}); | |||
|
|||
it('should crete instance even when no injector present', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crete
-> create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
019b4c1
to
e2a3a2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
You can preview 091c9e1 at https://pr23345-091c9e1.ngbuilds.io/. |
You can preview 019b4c1 at https://pr23345-019b4c1.ngbuilds.io/. |
You can preview e2a3a2f at https://pr23345-e2a3a2f.ngbuilds.io/. |
packages/core/src/di/defs.ts
Outdated
@@ -107,6 +121,7 @@ export function defineInjectable<T>(opts: { | |||
return { | |||
providedIn: (opts.providedIn as InjectorType<any>| 'root' | null | undefined) || null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no 'any' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
} 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() : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to be able to provide undefined
by using a special value (ie {}
) instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it, and decided that it is not worth the complexity. We can always added later.
@@ -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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useful comment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated comment.
@@ -135,6 +137,11 @@ export interface IterableDifferFactory { | |||
* | |||
*/ | |||
export class IterableDiffers { | |||
static ngInjectableDef: InjectableDef<IterableDiffers> = defineInjectable({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would compilation result lives in source code? Shouldn't it using @Injectable()
decorator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should probably be in the source code
You can preview 27674e3 at https://pr23345-27674e3.ngbuilds.io/. |
You can preview 685546e at https://pr23345-685546e.ngbuilds.io/. |
this PR changes the API surface. @mhevery let's discuss tomorrow before merging this. |
Discussed with @IgorMinar and we agreed to merge. |
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`
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
@@ -376,8 +376,10 @@ export interface InjectDecorator { | |||
|
|||
export declare const enum InjectFlags { | |||
Default = 0, | |||
SkipSelf = 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the order? It's a bitmask anyway o:
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This change changes:
directiveInject
instead ofinject
forDirective
sdi
as well asrender3
directiveInject
to matchinject
In prep forinject
anddirectiveInject
need bridging when used withComponent.provides
#23330Compiler portion of #23342
Prep for #23330
feat(ivy): support injection even if no injector present
inject
/directiveInject
sinceit is not possible to set using annotations.
Injector
is stored onLView
instead ofLInjector
datastructure because it can change only at
LView
level. (More efficient)ngInjectableDef
toIterableDiffers
so that existing tests canpass as well as enable
IterableDiffers
to be injectable withoutInjector