Skip to content

Commit

Permalink
fix(core): Allow modification of lifecycle hooks any time before boot…
Browse files Browse the repository at this point in the history
…strap

Currently we read lifecycle hooks eagerly during `ɵɵdefineComponent`. The result is that it is not possible to do any sort of meta-programing such as mixins or adding lifecycle hooks using custom decorators since any such code executes after `ɵɵdefineComponent` has extracted the lifecycle hooks from the prototype. Additionally the behavior is inconsistent between AOT and JIT mode. In JIT mode overriding lifecycle hooks is possible because the whole `ɵɵdefineComponent` is placed in getter which is executed lazily. This is because JIT mode must compile a template which can be specified as `templateURL` and those we are waiting for its resolution.

- `+` `ɵɵdefineComponent` becomes smaller as it no longer needs to copy lifecycle hooks from prototype to `ComponentDef`
- `-` `ɵɵNgOnChangesFeature` feature is now always included with the codebase as it is no longer tree shakable.

Previously we have read lifecycle hooks from prototype in the `ɵɵdefineComponent` so that lifecycle hook access would be monomorphic. This decision was made before we had `T*` data structures. By not reading the lifecycle hooks we are moving the megamorhic read form `ɵɵdefineComponent` to instructions. However, the reads happen on `firstTemplatePass` only and are subsequently cached in the `T*` data structures. The result is that the overall performance should be same (or slightly better as the intermediate `ComponentDef` has been removed)

- [ ] Remove `ɵɵNgOnChangesFeature` from compiler. (It will no longer be a feature.)
- [ ] Discuss the future of `Features` as they hinder meta-programing.

Fix angular#30497
  • Loading branch information
mhevery committed Mar 11, 2020
1 parent 5af0e75 commit c66cba3
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 82 deletions.
10 changes: 1 addition & 9 deletions packages/core/src/render3/definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -314,14 +314,6 @@ export function ɵɵdefineComponent<T>(componentDefinition: {
inputs: null !, // assigned in noSideEffects
outputs: null !, // assigned in noSideEffects
exportAs: componentDefinition.exportAs || null,
onChanges: null,
onInit: typePrototype.ngOnInit || null,
doCheck: typePrototype.ngDoCheck || null,
afterContentInit: typePrototype.ngAfterContentInit || null,
afterContentChecked: typePrototype.ngAfterContentChecked || null,
afterViewInit: typePrototype.ngAfterViewInit || null,
afterViewChecked: typePrototype.ngAfterViewChecked || null,
onDestroy: typePrototype.ngOnDestroy || null,
onPush: componentDefinition.changeDetection === ChangeDetectionStrategy.OnPush,
directiveDefs: null !, // assigned in noSideEffects
pipeDefs: null !, // assigned in noSideEffects
Expand Down Expand Up @@ -355,7 +347,7 @@ export function ɵɵdefineComponent<T>(componentDefinition: {
null;

return def as never;
});
}) as never;
}

/**
Expand Down
11 changes: 0 additions & 11 deletions packages/core/src/render3/features/inherit_definition_feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,6 @@ export function ɵɵInheritDefinitionFeature(definition: DirectiveDef<any>| Comp
const defData = (definition as ComponentDef<any>).data;
defData.animation = (defData.animation || []).concat(superDef.data.animation);
}

// Inherit hooks
// Assume super class inheritance feature has already run.
writeableDef.afterContentChecked =
writeableDef.afterContentChecked || superDef.afterContentChecked;
writeableDef.afterContentInit = definition.afterContentInit || superDef.afterContentInit;
writeableDef.afterViewChecked = definition.afterViewChecked || superDef.afterViewChecked;
writeableDef.afterViewInit = definition.afterViewInit || superDef.afterViewInit;
writeableDef.doCheck = definition.doCheck || superDef.doCheck;
writeableDef.onDestroy = definition.onDestroy || superDef.onDestroy;
writeableDef.onInit = definition.onInit || superDef.onInit;
}

// Run parent features
Expand Down
47 changes: 20 additions & 27 deletions packages/core/src/render3/features/ng_onchanges_feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,6 @@ import {SimpleChange, SimpleChanges} from '../../interface/simple_change';
import {EMPTY_OBJ} from '../empty';
import {DirectiveDef, DirectiveDefFeature} from '../interfaces/definition';

const PRIVATE_PREFIX = '__ngOnChanges_';

type OnChangesExpando = OnChanges & {
__ngOnChanges_: SimpleChanges|null|undefined;
// tslint:disable-next-line:no-any Can hold any value
[key: string]: any;
};

/**
* The NgOnChangesFeature decorates a component with support for the ngOnChanges
* lifecycle hook, so it should be included in any component that implements
Expand All @@ -41,12 +33,15 @@ type OnChangesExpando = OnChanges & {
*
* @codeGenApi
*/
export function ɵɵNgOnChangesFeature<T>(): DirectiveDefFeature {
return NgOnChangesFeatureImpl;
}

export function ɵɵNgOnChangesFeature<T>(definition: DirectiveDef<T>): void {
export function NgOnChangesFeatureImpl<T>(definition: DirectiveDef<T>) {
if (definition.type.prototype.ngOnChanges) {
definition.setInput = ngOnChangesSetInput;
(definition as{onChanges: Function}).onChanges = wrapOnChanges();
}
return wrapOnChangesHook_inPreviousChangesStorage;
}

// This option ensures that the ngOnChanges lifecycle hook will be inherited
Expand All @@ -55,26 +50,24 @@ export function ɵɵNgOnChangesFeature<T>(definition: DirectiveDef<T>): void {
// tslint:disable-next-line:no-toplevel-property-access
(ɵɵNgOnChangesFeature as DirectiveDefFeature).ngInherit = true;

function wrapOnChanges() {
return function wrapOnChangesHook_inPreviousChangesStorage(this: OnChanges) {
const simpleChangesStore = getSimpleChangesStore(this);
const current = simpleChangesStore && simpleChangesStore.current;
function wrapOnChangesHook_inPreviousChangesStorage(this: OnChanges) {
const simpleChangesStore = getSimpleChangesStore(this);
const current = simpleChangesStore && simpleChangesStore.current;

if (current) {
const previous = simpleChangesStore !.previous;
if (previous === EMPTY_OBJ) {
simpleChangesStore !.previous = current;
} else {
// New changes are copied to the previous store, so that we don't lose history for inputs
// which were not changed this time
for (let key in current) {
previous[key] = current[key];
}
if (current) {
const previous = simpleChangesStore !.previous;
if (previous === EMPTY_OBJ) {
simpleChangesStore !.previous = current;
} else {
// New changes are copied to the previous store, so that we don't lose history for inputs
// which were not changed this time
for (let key in current) {
previous[key] = current[key];
}
simpleChangesStore !.current = null;
this.ngOnChanges(current);
}
};
simpleChangesStore !.current = null;
this.ngOnChanges(current);
}
}

function ngOnChangesSetInput<T>(
Expand Down
54 changes: 31 additions & 23 deletions packages/core/src/render3/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@
* found in the LICENSE file at https://angular.io/license
*/

import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, DoCheck, OnChanges, OnDestroy, OnInit} from '../interface/lifecycle_hooks';
import {assertEqual, assertNotEqual} from '../util/assert';

import {assertFirstCreatePass} from './assert';
import {NgOnChangesFeatureImpl} from './features/ng_onchanges_feature';
import {DirectiveDef} from './interfaces/definition';
import {TNode} from './interfaces/node';
import {FLAGS, HookData, InitPhaseState, LView, LViewFlags, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TView} from './interfaces/view';
Expand All @@ -31,20 +32,23 @@ import {getCheckNoChangesMode} from './state';
export function registerPreOrderHooks(
directiveIndex: number, directiveDef: DirectiveDef<any>, tView: TView): void {
ngDevMode && assertFirstCreatePass(tView);
const {onChanges, onInit, doCheck} = directiveDef;
const {ngOnChanges, ngOnInit, ngDoCheck} =
directiveDef.type.prototype as OnChanges & OnInit & DoCheck;

if (onChanges) {
(tView.preOrderHooks || (tView.preOrderHooks = [])).push(directiveIndex, onChanges);
(tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [])).push(directiveIndex, onChanges);
if (ngOnChanges as Function | undefined) {
const wrappedOnChanges = NgOnChangesFeatureImpl(directiveDef);
(tView.preOrderHooks || (tView.preOrderHooks = [])).push(directiveIndex, wrappedOnChanges);
(tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [
])).push(directiveIndex, wrappedOnChanges);
}

if (onInit) {
(tView.preOrderHooks || (tView.preOrderHooks = [])).push(-directiveIndex, onInit);
if (ngOnInit) {
(tView.preOrderHooks || (tView.preOrderHooks = [])).push(0 - directiveIndex, ngOnInit);
}

if (doCheck) {
(tView.preOrderHooks || (tView.preOrderHooks = [])).push(directiveIndex, doCheck);
(tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [])).push(directiveIndex, doCheck);
if (ngDoCheck) {
(tView.preOrderHooks || (tView.preOrderHooks = [])).push(directiveIndex, ngDoCheck);
(tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [])).push(directiveIndex, ngDoCheck);
}
}

Expand Down Expand Up @@ -73,27 +77,31 @@ export function registerPostOrderHooks(tView: TView, tNode: TNode): void {
// hooks for projected components and directives must be called *before* their hosts.
for (let i = tNode.directiveStart, end = tNode.directiveEnd; i < end; i++) {
const directiveDef = tView.data[i] as DirectiveDef<any>;
if (directiveDef.afterContentInit) {
(tView.contentHooks || (tView.contentHooks = [])).push(-i, directiveDef.afterContentInit);
const lifecycleHooks: AfterContentInit&AfterContentChecked&AfterViewInit&AfterViewChecked&
OnDestroy = directiveDef.type.prototype;
const {ngAfterContentInit, ngAfterContentChecked, ngAfterViewInit, ngAfterViewChecked,
ngOnDestroy} = lifecycleHooks;

if (ngAfterContentInit) {
(tView.contentHooks || (tView.contentHooks = [])).push(-i, ngAfterContentInit);
}

if (directiveDef.afterContentChecked) {
(tView.contentHooks || (tView.contentHooks = [])).push(i, directiveDef.afterContentChecked);
(tView.contentCheckHooks || (tView.contentCheckHooks = [
])).push(i, directiveDef.afterContentChecked);
if (ngAfterContentChecked) {
(tView.contentHooks || (tView.contentHooks = [])).push(i, ngAfterContentChecked);
(tView.contentCheckHooks || (tView.contentCheckHooks = [])).push(i, ngAfterContentChecked);
}

if (directiveDef.afterViewInit) {
(tView.viewHooks || (tView.viewHooks = [])).push(-i, directiveDef.afterViewInit);
if (ngAfterViewInit) {
(tView.viewHooks || (tView.viewHooks = [])).push(-i, ngAfterViewInit);
}

if (directiveDef.afterViewChecked) {
(tView.viewHooks || (tView.viewHooks = [])).push(i, directiveDef.afterViewChecked);
(tView.viewCheckHooks || (tView.viewCheckHooks = [])).push(i, directiveDef.afterViewChecked);
if (ngAfterViewChecked) {
(tView.viewHooks || (tView.viewHooks = [])).push(i, ngAfterViewChecked);
(tView.viewCheckHooks || (tView.viewCheckHooks = [])).push(i, ngAfterViewChecked);
}

if (directiveDef.onDestroy != null) {
(tView.destroyHooks || (tView.destroyHooks = [])).push(i, directiveDef.onDestroy);
if (ngOnDestroy != null) {
(tView.destroyHooks || (tView.destroyHooks = [])).push(i, ngOnDestroy);
}
}
}
Expand Down
7 changes: 5 additions & 2 deletions packages/core/src/render3/instructions/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/
import {Injector} from '../../di';
import {ErrorHandler} from '../../error_handler';
import {DoCheck, OnChanges, OnInit} from '../../interface/lifecycle_hooks';
import {CUSTOM_ELEMENTS_SCHEMA, NO_ERRORS_SCHEMA, SchemaMetadata} from '../../metadata/schema';
import {ViewEncapsulation} from '../../metadata/view';
import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../../sanitization/sanitization';
Expand Down Expand Up @@ -1144,17 +1145,19 @@ export function resolveDirectives(
if (def.hostBindings !== null || def.hostAttrs !== null || def.hostVars !== 0)
tNode.flags |= TNodeFlags.hasHostBindings;

const lifeCycleHooks: OnChanges&OnInit&DoCheck = def.type.prototype;
// Only push a node index into the preOrderHooks array if this is the first
// pre-order hook found on this node.
if (!preOrderHooksFound && (def.onChanges || def.onInit || def.doCheck)) {
if (!preOrderHooksFound &&
(lifeCycleHooks.ngOnChanges || lifeCycleHooks.ngOnInit || lifeCycleHooks.ngDoCheck)) {
// We will push the actual hook function into this array later during dir instantiation.
// We cannot do it now because we must ensure hooks are registered in the same
// order that directives are created (i.e. injection order).
(tView.preOrderHooks || (tView.preOrderHooks = [])).push(tNode.index - HEADER_OFFSET);
preOrderHooksFound = true;
}

if (!preOrderCheckHooksFound && (def.onChanges || def.doCheck)) {
if (!preOrderCheckHooksFound && (lifeCycleHooks.ngOnChanges || lifeCycleHooks.ngDoCheck)) {
(tView.preOrderCheckHooks || (tView.preOrderCheckHooks = [
])).push(tNode.index - HEADER_OFFSET);
preOrderCheckHooksFound = true;
Expand Down
10 changes: 0 additions & 10 deletions packages/core/src/render3/interfaces/definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,16 +210,6 @@ export interface DirectiveDef<T> {
*/
readonly factory: FactoryFn<T>|null;

/* The following are lifecycle hooks for this component */
readonly onChanges: (() => void)|null;
readonly onInit: (() => void)|null;
readonly doCheck: (() => void)|null;
readonly afterContentInit: (() => void)|null;
readonly afterContentChecked: (() => void)|null;
readonly afterViewInit: (() => void)|null;
readonly afterViewChecked: (() => void)|null;
readonly onDestroy: (() => void)|null;

/**
* The features applied to this directive
*/
Expand Down

0 comments on commit c66cba3

Please sign in to comment.