From bbdea96a6679c84766fdb6cf66ae160b84523bc6 Mon Sep 17 00:00:00 2001 From: Pawel Kozlowski Date: Fri, 15 Dec 2017 14:59:17 +0100 Subject: [PATCH] refactor: remove import circular dependencies (#20855) This PR fixes a circular dependency among those files in Renderer3: `query` -> `di` -> `instructions` -> `query` -> ... Looking at the above dependencies the `di` -> `instructions` import is a problematic one. Previously `di` had an import from `instructions` since we can known about "current node" only in `instructions` (and we need "current node" to create node injector instances). This commit refactors the code in the way that functions in the `di` file don't depend on any info stored module-global variables in `instructions`. PR Close #20855 --- packages/core/src/render3/di.ts | 144 +++++++++++++++----- packages/core/src/render3/index.ts | 4 +- packages/core/src/render3/instructions.ts | 155 +++++++++------------- packages/core/src/render3/query.ts | 5 +- packages/core/test/render3/di_spec.ts | 4 +- 5 files changed, 184 insertions(+), 128 deletions(-) diff --git a/packages/core/src/render3/di.ts b/packages/core/src/render3/di.ts index 74dc9004b1..59b1fc220d 100644 --- a/packages/core/src/render3/di.ts +++ b/packages/core/src/render3/di.ts @@ -10,12 +10,103 @@ // correctly implementing its interfaces for backwards compatibility. import * as viewEngine from '../core'; -import {BLOOM_SIZE, NG_ELEMENT_ID, getOrCreateNodeInjector} from './instructions'; import {LContainer, LElement, LNodeFlags, LNodeInjector} from './l_node'; import {ComponentTemplate, DirectiveDef} from './public_interfaces'; import {notImplemented, stringify} from './util'; +/** + * If a directive is diPublic, bloomAdd sets a property on the instance with this constant as + * the key and the directive's unique ID as the value. This allows us to map directives to their + * bloom filter bit for DI. + */ +const NG_ELEMENT_ID = '__NG_ELEMENT_ID__'; + +/** + * The number of slots in each bloom filter (used by DI). The larger this number, the fewer + * directives that will share slots, and thus, the fewer false positives when checking for + * the existence of a directive. + */ +const BLOOM_SIZE = 128; + +/** Counter used to generate unique IDs for directives. */ +let nextNgElementId = 0; + +/** + * Registers this directive as present in its node's injector by flipping the directive's + * corresponding bit in the injector's bloom filter. + * + * @param injector The node injector in which the directive should be registered + * @param type The directive to register + */ +export function bloomAdd(injector: LNodeInjector, type: viewEngine.Type): void { + let id: number|undefined = (type as any)[NG_ELEMENT_ID]; + + // Set a unique ID on the directive type, so if something tries to inject the directive, + // we can easily retrieve the ID and hash it into the bloom bit that should be checked. + if (id == null) { + id = (type as any)[NG_ELEMENT_ID] = nextNgElementId++; + } + + // We only have BLOOM_SIZE (128) slots in our bloom filter (4 buckets * 32 bits each), + // so all unique IDs must be modulo-ed into a number from 0 - 127 to fit into the filter. + // This means that after 128, some directives will share slots, leading to some false positives + // when checking for a directive's presence. + const bloomBit = id % BLOOM_SIZE; + + // Create a mask that targets the specific bit associated with the directive. + // JS bit operations are 32 bits, so this will be a number between 2^0 and 2^31, corresponding + // to bit positions 0 - 31 in a 32 bit integer. + const mask = 1 << bloomBit; + + // Use the raw bloomBit number to determine which bloom filter bucket we should check + // e.g: bf0 = [0 - 31], bf1 = [32 - 63], bf2 = [64 - 95], bf3 = [96 - 127] + if (bloomBit < 64) { + if (bloomBit < 32) { + // Then use the mask to flip on the bit (0-31) associated with the directive in that bucket + injector.bf0 |= mask; + } else { + injector.bf1 |= mask; + } + } else { + if (bloomBit < 96) { + injector.bf2 |= mask; + } else { + injector.bf3 |= mask; + } + } +} + +/** + * Creates (or gets an existing) injector for a given element or container. + * + * @param {LElement | LContainer} node for which an injector should be retrieved / created. + * @returns {LNodeInjector} Node injector + */ +export function getOrCreateNodeInjectorForNode(node: LElement | LContainer): LNodeInjector { + const nodeInjector = node.nodeInjector; + const parentInjector = node.parent && node.parent.nodeInjector; + if (nodeInjector != parentInjector) { + return nodeInjector !; + } + return node.nodeInjector = { + parent: parentInjector, + node: node, + bf0: 0, + bf1: 0, + bf2: 0, + bf3: 0, + cbf0: parentInjector == null ? 0 : parentInjector.cbf0 | parentInjector.bf0, + cbf1: parentInjector == null ? 0 : parentInjector.cbf1 | parentInjector.bf1, + cbf2: parentInjector == null ? 0 : parentInjector.cbf2 | parentInjector.bf2, + cbf3: parentInjector == null ? 0 : parentInjector.cbf3 | parentInjector.bf3, + injector: null, + templateRef: null, + viewContainerRef: null, + elementRef: null + }; +} + /** Injection flags for DI. */ export const enum InjectFlags { /** Dependency is not required. Null will be injected if there is no provider for the dependency. @@ -40,6 +131,16 @@ function createInjectionError(text: string, token: any) { return new Error(`ElementInjector: ${text} [${stringify(token)}]`); } +/** + * Makes a directive public to the DI system by adding it to an injector's bloom filter. + * + * @param di The node injector in which a directive will be added + * @param def The definition of the directive to be made public + */ +export function diPublicInInjector(di: LNodeInjector, def: DirectiveDef): void { + bloomAdd(di, def.type); +} + /** * Searches for an instance of the given directive type up the injector tree and returns * that instance if found. @@ -52,23 +153,13 @@ function createInjectionError(text: string, token: any) { * If not found, it will propagate up to the next parent injector until the token * is found or the top is reached. * - * Usage example (in factory function): - * - * class SomeDirective { - * constructor(directive: DirectiveA) {} - * - * static ngDirectiveDef = defineDirective({ - * type: SomeDirective, - * factory: () => new SomeDirective(inject(DirectiveA)) - * }); - * } - * + * @param di Node injector where the search should start * @param token The directive type to search for * @param flags Injection flags (e.g. CheckParent) * @returns The instance found */ -export function inject(token: viewEngine.Type, flags?: InjectFlags): T { - const di = getOrCreateNodeInjector(); +export function getOrCreateInjectable( + di: LNodeInjector, token: viewEngine.Type, flags?: InjectFlags): T { const bloomHash = bloomHashBit(token); // If the token has a bloom hash, then it is a directive that is public to the injection system @@ -161,7 +252,7 @@ function bloomHashBit(type: viewEngine.Type): number|null { * injectors, a 0 in the bloom bit indicates that the parents definitely do not contain * the directive and do not need to be checked. * - * @param injector The starting injector to check + * @param injector The starting node injector to check * @param bloomBit The bit to check in each injector's bloom filter * @returns An injector that might have the directive */ @@ -201,24 +292,16 @@ export function bloomFindPossibleInjector( } /** - * Creates an ElementRef for a given node and stores it on the injector. + * Creates an ElementRef for a given node injector and stores it on the injector. * Or, if the ElementRef already exists, retrieves the existing ElementRef. * + * @param di The node injector where we should store a created ElementRef * @returns The ElementRef instance to use */ -export function injectElementRefForNode(node?: LElement | LContainer): viewEngine.ElementRef { - let di = getOrCreateNodeInjector(node); +export function getOrCreateElementRef(di: LNodeInjector): viewEngine.ElementRef { return di.elementRef || (di.elementRef = new ElementRef(di.node.native)); } -/** - * Creates an ElementRef and stores it on the injector. Or, if the ElementRef already - * exists, retrieves the existing ElementRef. - * - * @returns The ElementRef instance to use - */ -export const injectElementRef: () => viewEngine.ElementRef = injectElementRefForNode; - /** A ref to a node's native element. */ class ElementRef implements viewEngine.ElementRef { readonly nativeElement: any; @@ -229,16 +312,16 @@ class ElementRef implements viewEngine.ElementRef { * Creates a TemplateRef and stores it on the injector. Or, if the TemplateRef already * exists, retrieves the existing TemplateRef. * + * @param di The node injector where we should store a created TemplateRef * @returns The TemplateRef instance to use */ -export function injectTemplateRef(): viewEngine.TemplateRef { - let di = getOrCreateNodeInjector(); +export function getOrCreateTemplateRef(di: LNodeInjector): viewEngine.TemplateRef { const data = (di.node as LContainer).data; if (data === null || data.template === null) { throw createInjectionError('Directive does not have a template.', null); } return di.templateRef || - (di.templateRef = new TemplateRef(injectElementRef(), data.template)); + (di.templateRef = new TemplateRef(getOrCreateElementRef(di), data.template)); } /** A ref to a particular template. */ @@ -258,8 +341,7 @@ class TemplateRef implements viewEngine.TemplateRef { * * @returns The ViewContainerRef instance to use */ -export function injectViewContainerRef(): viewEngine.ViewContainerRef { - let di = getOrCreateNodeInjector(); +export function getOrCreateContainerRef(di: LNodeInjector): viewEngine.ViewContainerRef { return di.viewContainerRef || (di.viewContainerRef = new ViewContainerRef(di.node as LContainer)); } diff --git a/packages/core/src/render3/index.ts b/packages/core/src/render3/index.ts index 33d03d6fca..f5373e1440 100644 --- a/packages/core/src/render3/index.ts +++ b/packages/core/src/render3/index.ts @@ -7,7 +7,6 @@ */ import {createComponentRef, detectChanges, getHostElement, markDirty, renderComponent} from './component'; -import {inject, injectElementRef, injectTemplateRef, injectViewContainerRef} from './di'; import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef, DirectiveDefFlags, NgOnChangesFeature, PublicFeature, defineComponent, defineDirective} from './public_interfaces'; // Naming scheme: @@ -20,6 +19,8 @@ import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef, DirectiveD // - lower case for closing: c(containerEnd), e(elementEnd), v(viewEnd) // clang-format off export { + inject, injectElementRef, injectTemplateRef, injectViewContainerRef, + LifecycleHook, NO_CHANGE as NC, @@ -69,7 +70,6 @@ export { } from './instructions'; // clang-format on export {QueryList} from './query'; -export {inject, injectElementRef, injectTemplateRef, injectViewContainerRef}; export { ComponentDef, ComponentTemplate, diff --git a/packages/core/src/render3/instructions.ts b/packages/core/src/render3/instructions.ts index 9bfee04fdf..95c7508458 100644 --- a/packages/core/src/render3/instructions.ts +++ b/packages/core/src/render3/instructions.ts @@ -8,7 +8,7 @@ import './ng_dev_mode'; -import {Type} from '../core'; +import {ElementRef, TemplateRef, Type, ViewContainerRef} from '../core'; import {assertEqual, assertLessThan, assertNotEqual, assertNotNull} from './assert'; import {ContainerState, CssSelector, ProjectionState, QueryState, ViewState} from './interfaces'; @@ -19,6 +19,7 @@ import {assertNodeType} from './node_assert'; import {appendChild, insertChild, insertView, processProjectedNode, removeView} from './node_manipulation'; import {isNodeMatchingSelector} from './node_selector_matcher'; import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef} from './public_interfaces'; +import {InjectFlags, diPublicInInjector, getOrCreateNodeInjectorForNode, getOrCreateElementRef, getOrCreateTemplateRef, getOrCreateContainerRef, getOrCreateInjectable} from './di'; import {QueryList, QueryState_} from './query'; import {RComment, RElement, RText, Renderer3, RendererFactory3, ProceduralRenderer3, ObjectOrientedRenderer3, RendererStyleFlags3} from './renderer'; import {isDifferent, stringify} from './util'; @@ -38,23 +39,6 @@ export const enum LifecycleHook {ON_INIT = 1, ON_DESTROY = 2, ON_CHANGES = 4} */ export const NG_HOST_SYMBOL = '__ngHostLNode__'; -/** - * If a directive is diPublic, bloomAdd sets a property on the instance with this constant as - * the key and the directive's unique ID as the value. This allows us to map directives to their - * bloom filter bit for DI. - */ -export const NG_ELEMENT_ID = '__NG_ELEMENT_ID__'; - -/** - * The number of slots in each bloom filter (used by DI). The larger this number, the fewer - * directives that will share slots, and thus, the fewer false positives when checking for - * the existence of a directive. - */ -export const BLOOM_SIZE = 128; - -/** Counter used to generate unique IDs for directives. */ -let nextNgElementId = 0; - /** * This property gets set before entering a template. * @@ -321,77 +305,75 @@ export function renderComponentOrTemplate( } } +export function getOrCreateNodeInjector(): LNodeInjector { + ngDevMode && assertPreviousIsParent(); + return getOrCreateNodeInjectorForNode(previousOrParentNode as LElement | LContainer); +} + /** - * Registers this directive as present in its node's injector by flipping the directive's - * corresponding bit in the injector's bloom filter. + * Makes a directive public to the DI system by adding it to an injector's bloom filter. * - * @param injector The injector in which the directive should be registered - * @param type The directive to register + * @param def The definition of the directive to be made public */ -export function bloomAdd(injector: LNodeInjector, type: Type): void { - let id: number|undefined = (type as any)[NG_ELEMENT_ID]; - - // Set a unique ID on the directive type, so if something tries to inject the directive, - // we can easily retrieve the ID and hash it into the bloom bit that should be checked. - if (id == null) { - id = (type as any)[NG_ELEMENT_ID] = nextNgElementId++; - } - - // We only have BLOOM_SIZE (128) slots in our bloom filter (4 buckets * 32 bits each), - // so all unique IDs must be modulo-ed into a number from 0 - 127 to fit into the filter. - // This means that after 128, some directives will share slots, leading to some false positives - // when checking for a directive's presence. - const bloomBit = id % BLOOM_SIZE; - - // Create a mask that targets the specific bit associated with the directive. - // JS bit operations are 32 bits, so this will be a number between 2^0 and 2^31, corresponding - // to bit positions 0 - 31 in a 32 bit integer. - const mask = 1 << bloomBit; - - // Use the raw bloomBit number to determine which bloom filter bucket we should check - // e.g: bf0 = [0 - 31], bf1 = [32 - 63], bf2 = [64 - 95], bf3 = [96 - 127] - if (bloomBit < 64) { - if (bloomBit < 32) { - // Then use the mask to flip on the bit (0-31) associated with the directive in that bucket - injector.bf0 |= mask; - } else { - injector.bf1 |= mask; - } - } else { - if (bloomBit < 96) { - injector.bf2 |= mask; - } else { - injector.bf3 |= mask; - } - } +export function diPublic(def: DirectiveDef): void { + diPublicInInjector(getOrCreateNodeInjector(), def); } -export function getOrCreateNodeInjector(node?: LElement | LContainer): LNodeInjector { - ngDevMode && !node && assertPreviousIsParent(); - node = node || previousOrParentNode as LElement | LContainer; - const nodeInjector = node.nodeInjector; - const parentInjector = node.parent && node.parent.nodeInjector; - if (nodeInjector != parentInjector) { - return nodeInjector !; - } - return node.nodeInjector = { - parent: parentInjector, - node: node, - bf0: 0, - bf1: 0, - bf2: 0, - bf3: 0, - cbf0: parentInjector == null ? 0 : parentInjector.cbf0 | parentInjector.bf0, - cbf1: parentInjector == null ? 0 : parentInjector.cbf1 | parentInjector.bf1, - cbf2: parentInjector == null ? 0 : parentInjector.cbf2 | parentInjector.bf2, - cbf3: parentInjector == null ? 0 : parentInjector.cbf3 | parentInjector.bf3, - injector: null, - templateRef: null, - viewContainerRef: null, - elementRef: null - }; +/** + * Searches for an instance of the given directive type up the injector tree and returns + * that instance if found. + * + * If not found, it will propagate up to the next parent injector until the token + * is found or the top is reached. + * + * Usage example (in factory function): + * + * class SomeDirective { + * constructor(directive: DirectiveA) {} + * + * static ngDirectiveDef = defineDirective({ + * type: SomeDirective, + * factory: () => new SomeDirective(inject(DirectiveA)) + * }); + * } + * + * @param token The directive type to search for + * @param flags Injection flags (e.g. CheckParent) + * @returns The instance found + */ +export function inject(token: Type, flags?: InjectFlags): T { + return getOrCreateInjectable(getOrCreateNodeInjector(), token, flags); } +/** + * Creates an ElementRef and stores it on the injector. + * Or, if the ElementRef already exists, retrieves the existing ElementRef. + * + * @returns The ElementRef instance to use + */ +export function injectElementRef(): ElementRef { + return getOrCreateElementRef(getOrCreateNodeInjector()); +} + +/** + * Creates a TemplateRef and stores it on the injector. Or, if the TemplateRef already + * exists, retrieves the existing TemplateRef. + * + * @returns The TemplateRef instance to use + */ +export function injectTemplateRef(): TemplateRef { + return getOrCreateTemplateRef(getOrCreateNodeInjector()); +} + +/** + * Creates a ViewContainerRef and stores it on the injector. Or, if the ViewContainerRef + * already exists, retrieves the existing ViewContainerRef. + * + * @returns The ViewContainerRef instance to use + */ +export function injectViewContainerRef(): ViewContainerRef { + return getOrCreateContainerRef(getOrCreateNodeInjector()); +} ////////////////////////// //// Element @@ -938,15 +920,6 @@ function generateInitialInputs( return initialInputData; } -/** - * Makes a directive public to the DI system by adding it to an injector's bloom filter. - * - * @param def The definition of the directive to be made public - */ -export function diPublic(def: DirectiveDef): void { - bloomAdd(getOrCreateNodeInjector(), def.type); -} - /** * Accepts a lifecycle hook type and determines when and how the related lifecycle hook * callback should run. diff --git a/packages/core/src/render3/query.ts b/packages/core/src/render3/query.ts index 421bc79c52..5035f32482 100644 --- a/packages/core/src/render3/query.ts +++ b/packages/core/src/render3/query.ts @@ -11,7 +11,7 @@ import {Observable} from 'rxjs/Observable'; import * as viewEngine from '../core'; import {assertNotNull} from './assert'; -import {injectElementRefForNode} from './di'; +import {getOrCreateElementRef, getOrCreateNodeInjectorForNode} from './di'; import {QueryState} from './interfaces'; import {LContainer, LElement, LNode, LNodeFlags, LView} from './l_node'; import {DirectiveDef} from './public_interfaces'; @@ -117,7 +117,8 @@ function add(predicate: QueryPredicate| null, node: LNode) { const selector = predicate.selector !; for (let i = 0; i < selector.length; i++) { if (selector[i] === staticData.localName) { - predicate.values.push(injectElementRefForNode(node as LElement | LContainer)); + predicate.values.push(getOrCreateElementRef( + getOrCreateNodeInjectorForNode(node as LElement | LContainer))); } } } diff --git a/packages/core/test/render3/di_spec.ts b/packages/core/test/render3/di_spec.ts index 1264c96813..61c41c7f52 100644 --- a/packages/core/test/render3/di_spec.ts +++ b/packages/core/test/render3/di_spec.ts @@ -8,9 +8,9 @@ import {ElementRef, TemplateRef, ViewContainerRef} from '@angular/core'; -import {bloomFindPossibleInjector} from '../../src/render3/di'; +import {bloomAdd, bloomFindPossibleInjector} from '../../src/render3/di'; import {C, D, E, PublicFeature, T, V, b, b2, c, cR, cr, defineDirective, e, inject, injectElementRef, injectTemplateRef, injectViewContainerRef, t, v} from '../../src/render3/index'; -import {bloomAdd, createLNode, createViewState, enterView, getOrCreateNodeInjector, leaveView} from '../../src/render3/instructions'; +import {createLNode, createViewState, enterView, getOrCreateNodeInjector, leaveView} from '../../src/render3/instructions'; import {LNodeFlags, LNodeInjector} from '../../src/render3/l_node'; import {renderToHtml} from './render_util';