feat(ivy): support injecting ChangeDetectorRef #22469
Conversation
|
You can preview 4b79e33 at https://pr22469-4b79e33.ngbuilds.io/. |
|
You can preview bc2e407 at https://pr22469-bc2e407.ngbuilds.io/. |
|
You can preview 8849439 at https://pr22469-8849439.ngbuilds.io/. |
|
You can preview f40ca1e at https://pr22469-f40ca1e.ngbuilds.io/. |
kara
added
action: review
packages/core/src/render3/di.ts
Outdated
| return di.changeDetectorRef = getOrCreateHostChangeDetector(currentNode.view.node); | ||
| } else if ((currentNode.flags & LNodeFlags.TYPE_MASK) === LNodeFlags.Element) { | ||
| // if it's an element node with data, it's a component and context will be set later | ||
| return di.changeDetectorRef = createViewRef(null); |
There was a problem hiding this comment.
So we call createViewRef(null) because we don't have the instance just yet, that makes sense. But what if someone does injector.get(ViewRef)? in that case I assume we would call getOrCreateChangeDetectorRef and we would incorrectly create a ViewRef with null. I think we should have getOrCreateChangeDetectorRef take the context in the args. Then injectChangeDetectorRefcan invokegetOrCreateChangeDetectorRefwithnullandinjector.get(ViewRef)can invoke it with correctcontext`.
| @@ -580,9 +590,9 @@ export function locateHostElement( | |||
| * @param rNode Render host element. | |||
| * @param def ComponentDef | |||
There was a problem hiding this comment.
add @returns documentation
| /** Sets the context for a ChangeDetectorRef to the given instance. */ | ||
| export function initChangeDetectorIfExisting(injector: LInjector | null, instance: any): void { | ||
| if (injector && injector.changeDetectorRef != null) { | ||
| (injector.changeDetectorRef as any)._setComponentContext(instance); |
There was a problem hiding this comment.
injector.changeDetectorRef as ViewRef<any> would be better.
| it('should inject host component ChangeDetectorRef into directives on containers', () => { | ||
| class IfDirective { | ||
| /* @Input */ | ||
| if |
There was a problem hiding this comment.
clang-format thinks that if is a keyword here and it messes up. Can you come up with a name which is not a keyword? MyIfDirictive => myIf?
There was a problem hiding this comment.
doh! How did I miss that.
|
You can preview 48fd28f at https://pr22469-48fd28f.ngbuilds.io/. |
mhevery
removed
the
action: review
|
Hi @kara! This PR has merge conflicts due to recent upstream merges. |
|
You can preview c2c58ac at https://pr22469-c2c58ac.ngbuilds.io/. |
|
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 PR adds basic support for injecting the correct
ChangeDetectorRef. This will pave the way for supporting and testing ChangeDetectorRef's various methods, but this PR does not add any of those methods.Notes:
ChangeDetectorRef, what you get in practice is aViewRef(which implementsEmbeddedViewRef). I've preserved that for backwards compatibility.EmbeddedViewRefimplementations in render3: one indi.tsand one incomponent.ts. I've deleted both and combined their logic in a new file (view_ref.ts). ThedetectChangesarg has been removed until it can be properly supported/tested in a follow-up PR.ChangeDetectorRef, the context cannot be set at creation time because the context is the component instance itself (circular dependency). In this implementation, the context is set inelementStartafter the component instance is created.ChangeDetectorRef2+ times.TODO in follow-up PRs:
detectChangesandmarkForCheckcontainingDirtyflag and associated function