feat(ivy): add afterContentInit and afterContentChecked to render3 #21650
Conversation
kara
added
action: review
| @@ -40,8 +40,8 @@ export const enum LifecycleHook { | |||
| ON_INIT = 1, | |||
| ON_DESTROY = 2, | |||
| ON_CHANGES = 4, | |||
| AFTER_VIEW_INIT = 8, | |||
| AFTER_VIEW_CHECKED = 16 | |||
| AFTER_INIT = 8, | |||
There was a problem hiding this comment.
Why are we removing VIEW? We have AFTER_VIEW_INIT and AFTER_CONTENT_INIT
There was a problem hiding this comment.
Generalizing it so we can use the same code for content hooks and view hooks in executeHooksAndRemoveInits. We could also pass through the hook, but it would require creating two more hooks just for that check when they are treated the same.
|
|
||
| for (let i = start, end = start + size; i < end; i++) { | ||
| const instance = data[i]; | ||
| if (instance.ngAfterContentInit != null) { |
There was a problem hiding this comment.
so this is a megamorphic property read and we will do this every time we are creating the component. A better way is to store Component.prototype.ngAfterContentInit in TView that way we are doing the megamorphic property read only once per template. We than use the information from the TView to enqueue the instance into the contentHooks
| (contentHooks || (currentView.contentHooks = contentHooks = [ | ||
| ])).push(LifecycleHook.AFTER_INIT, instance.ngAfterContentInit, instance); | ||
| } | ||
| if (instance.ngAfterContentChecked != null) { |
| (contentHooks || (currentView.contentHooks = contentHooks = [ | ||
| ])).push(LifecycleHook.AFTER_CHECKED, instance.ngAfterContentChecked, instance); | ||
| } | ||
| if (instance.ngOnDestroy != null) { |
| * Calls all afterContentInit and afterContentChecked hooks for the view, then splices | ||
| * out afterContentInit hooks to prep for the next run in update mode. | ||
| */ | ||
| function executeContentHooks(): void { |
There was a problem hiding this comment.
could we move this into a file out of instructions.ts since it is getting long? I suggest new file hooks.ts?
(we will have to pass in contentHooks and currentView)
| * @param arr The array in which the hooks are found | ||
| * @param startIndex The index at which to start calling hooks | ||
| */ | ||
| function executeHooksAndRemoveInits(arr: any[], startIndex: number): void { |
There was a problem hiding this comment.
Same here move to different file.
| * out afterContentInit hooks to prep for the next run in update mode. | ||
| */ | ||
| function executeContentHooks(): void { | ||
| if (contentHooks == null || currentView.contentHooksCalled) return; |
There was a problem hiding this comment.
I have really hard time with early returns. Could you rewire it in form of a normal if () {} form?
| * adding to the code size), so it needs to be able to check whether or not they should | ||
| * be called. | ||
| */ | ||
| contentHooksCalled: boolean; |
There was a problem hiding this comment.
Sorry I don't understand why we need this. Can you explain what would happen if we did not have this?
There was a problem hiding this comment.
Then every Comp.r() call would call content hooks, and they'd be called multiple times per change detection run (once for each component in that view). So if you had:
Comp.r(1,0);
Comp.r(3,2);
The first r() call should run content hooks. The second one should not.
kara
force-pushed
the
afterContentInit
branch
2 times, most recently
from
0fb6253
to
7c9a0cd
Compare
kara
added
the
action: cleanup
kara
force-pushed
the
afterContentInit
branch
3 times, most recently
from
e28f10e
to
03216e9
Compare
kara
force-pushed
the
afterContentInit
branch
3 times, most recently
from
6b08bab
to
81de6c4
Compare
|
You can preview f7e09e2 at https://pr21650-f7e09e2.ngbuilds.io/. |
|
You can preview 6b08bab at https://pr21650-6b08bab.ngbuilds.io/. |
kara
force-pushed
the
afterContentInit
branch
from
81de6c4
to
11787b6
Compare
|
You can preview 81de6c4 at https://pr21650-81de6c4.ngbuilds.io/. |
kara
force-pushed
the
afterContentInit
branch
from
11787b6
to
e8cbff5
Compare
|
You can preview 11787b6 at https://pr21650-11787b6.ngbuilds.io/. |
|
You can preview e8cbff5 at https://pr21650-e8cbff5.ngbuilds.io/. |
kara
force-pushed
the
afterContentInit
branch
from
e8cbff5
to
9af98ea
Compare
|
You can preview 9af98ea at https://pr21650-9af98ea.ngbuilds.io/. |
|
You can preview 3d16302 at https://pr21650-3d16302.ngbuilds.io/. |
|
You can preview 2be9525 at https://pr21650-2be9525.ngbuilds.io/. |
kara
force-pushed
the
afterContentInit
branch
from
2be9525
to
2d77402
Compare
kara
removed
the
action: cleanup
Change compiler to reflect changes made in angular#21650
Change compiler to reflect changes made in angular#21650 PR Close angular#21862
Change compiler to reflect changes made in angular#21650 PR Close angular#21862
|
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 support for the
afterContentInitandafterContentCheckedhooks to render3. It also adds some missing tests for how projected components will work with other lifecycle hooks.Update:
This PR now also rewrites lifecycle hooks to remove the
l()instruction. We now queue all hooks on theTViewas we build up the node tree and execute them implicitly incomponentRefreshand view end.Notes:
typehas been added back toDirectiveDef. This allows us to copy over lifecycle hooks from the type duringdefineComponent. This is a megamorphic lookup, but it only runs once per component and it saves us from conducting a megamorphic lookup for each component instance.rhas been removed from theDirectiveDef, as it no longer has a purpose. Previously, it contained view hook declarations, but now that these are implicit, we can just callcomponentRefreshdirectly.cc @jelbourn