Skip to content
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(core): add ngAfterViewInit and ngAfterViewChecked support to ren… #21266

Closed
wants to merge 1 commit into from

Conversation

kara
Copy link
Contributor

@kara kara commented Jan 2, 2018

This PR adds support for the ngAfterViewInit and ngAfterViewChecked lifecycle hooks in render3. These hooks cannot simply be called inside the component refresh function because that would result in some ngAfterViewInit hooks being called prematurely (see correct order here). To fix this, the view hook functions are queued and only called once all the components below the current component in the tree have been processed.

Notes:

  • The view hook functions are stored in the data array, so we need to keep track of the viewHookStartIndex (the point in the data array after which only view hooks are stored). Storing this index is cheaper than creating a separate viewHooks array because arrays have 70 bytes each in memory overhead (which would be 70 bytes cost for every view).
  • We are storing creationMode on the ViewState again. Previously, we were using the existence of data as an implicit marker of creation mode, but this doesn't work for embedded views. When exiting a child view back into the parent view, data will be defined and creationMode will be improperly reported as false. This is important to fix now because if creation mode is false, the view hook array won't be built up correctly.
  • Any ngAfterViewInit hooks are spliced out as soon as they are invoked (ngAfterViewChecked hooks stay present). This will give us a performance hit in creation mode, but then we avoid iterating over dead init hooks in every change detection run after that.
  • We aren't actually using Array.splice, as it's too expensive. Instead we are manually shifting AfterViewChecked hooks down as we go through and truncating once at the end.

@mary-poppins
Copy link

You can preview bc8e9b0 at https://pr21266-bc8e9b0.ngbuilds.io/.

@mhevery mhevery self-requested a review January 3, 2018 17:49
lifecycle: LifecycleHook.AFTER_VIEW_CHECKED, self: any, method: Function): void;
export function lifecycle(lifecycle: LifecycleHook): boolean;
export function lifecycle(lifecycle: LifecycleHook, self?: any, method?: Function): boolean {
if (lifecycle & LifecycleHook.ON_INIT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you change it from === to &?

} else if (
creationMode &&
(lifecycle & LifecycleHook.AFTER_VIEW_INIT || lifecycle & LifecycleHook.AFTER_VIEW_CHECKED)) {
if (typeof viewHookStartIndex !== 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how abut == null that may be clearer than typeof

while (index < data.length) {
data[index + 1].call(data[index + 2]);
// Splice out init hooks after first run so we don't have to keep iterating over them
data[index]&LifecycleHook.AFTER_VIEW_INIT ? data.splice(index, 3) : index += 3;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repeated splice may get expensive. What about compacting the array and than truncating it?

Compacting means that you copy value from higher location to lower in essence moving the higher bits lower and that you just truncate it. (in essence that is what splice does, but we will do it only once where splice will do it once per item.)

}
return false;
}

/** Iterates over view hook functions and calls them. */
export function executeViewHooks(): void {
if (typeof viewHookStartIndex !== 'number') return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!== null

@kara kara self-assigned this Jan 3, 2018
@mhevery mhevery added the area: core Issues related to the framework runtime label Jan 4, 2018
@kara kara force-pushed the afterViewInit branch 2 times, most recently from 1dd8036 to 34a4acd Compare January 8, 2018 23:31
@mary-poppins
Copy link

You can preview 1dd8036 at https://pr21266-1dd8036.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 34a4acd at https://pr21266-34a4acd.ngbuilds.io/.

@kara kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 8, 2018
@mary-poppins
Copy link

You can preview b1d0c37 at https://pr21266-b1d0c37.ngbuilds.io/.

@kara kara added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 9, 2018
@mary-poppins
Copy link

You can preview 40f7a4f at https://pr21266-40f7a4f.ngbuilds.io/.

@alexeagle
Copy link
Contributor

@kara kara closed this in 3db91ff Jan 9, 2018
@mary-poppins
Copy link

You can preview 3a9b742 at https://pr21266-3a9b742.ngbuilds.io/.

@kara kara added comp: ivy and removed area: core Issues related to the framework runtime labels Jan 10, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants