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(ivy): allow combined context discovery for components, directives and elements #25754

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@matsko
Member

matsko commented Aug 30, 2018

No description provided.

@googlebot googlebot added the cla: yes label Aug 30, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 30, 2018

@matsko matsko changed the title from refactor(ivy): use monkey patching for context on components and elements to feat(ivy): allow combined context discovery for components, directives and elements Aug 30, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 30, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 30, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 30, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 31, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 31, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 31, 2018

@matsko

This comment has been minimized.

@mary-poppins

This comment has been minimized.

mary-poppins commented Aug 31, 2018

@matsko matsko requested a review from mhevery Aug 31, 2018

/** The component\'s view data */
lViewData: LViewData;
/** * The index of the LElementNode within the view data array */

This comment has been minimized.

@trotyl

trotyl Aug 31, 2018

Contributor

Extra *?

export function getContext(target: any): Context|null {
let context = target[MONKEY_PATCH_KEY_NAME] as Context | LViewData | null;
if (context) {
// only when its an array is it considered a lViewData instance

This comment has been minimized.

@trotyl

trotyl Aug 31, 2018

Contributor

its -> it's?

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Typo: a lViewData -> an LViewData

/** Locates the directive within the given LViewData and returns the matching index */
function findViaDirective(viewData: LViewData, directive: any): number {
// if a directive is monkey patched then it will (by default)
// have a referene to the lViewData of the current view. The

This comment has been minimized.

@trotyl

trotyl Aug 31, 2018

Contributor

Typo in referene?

}
function assertDomElement(element: any) {
assertEqual(element.nodeType, 1, 'The provided value must be an instance of a HTMLElement');

This comment has been minimized.

@trotyl

trotyl Aug 31, 2018

Contributor

a HTMLElement -> an HTMLElement? H: [eitʃ]

@@ -85,7 +86,7 @@ class Render3DebugContext implements DebugContext {
const currentNode = this.view[this.nodeIndex];
for (let dirIndex = 0; dirIndex < directives.length; dirIndex++) {
const directive = directives[dirIndex];
if (directive[NG_HOST_SYMBOL] === currentNode) {
if (getLElementNode(directive) === currentNode) {

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

This is an awfully convoluted way to get a list of directive constructors. I guess @pkozlowski-opensource Will fix that when he will rewrite the TestBed, but it hurts my eyes.

What this method is trying to do is to get a list of Tokens associated with the current Node. So you should be able to do this by going to TView and getting the list. No need to involve the getContext to do this.

@kara can provide more details on how to do this.

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Yeah, there's an easier way to do this. The node's TNode has the range of directive indices, so you should only need to check TView.directives at that range to get the tokens.

This comment has been minimized.

@matsko

matsko Sep 5, 2018

Member

This will get fixed up in a follow-up commit.

}

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

revert

import * as di from './di';
import {NG_HOST_SYMBOL, _getViewData} from './instructions';
import {_getViewData} from './instructions';

This comment was marked as outdated.

@mhevery

mhevery Aug 31, 2018

Member

unused import.

This comment was marked as outdated.

@matsko

matsko Sep 5, 2018

Member

There's nothing here that's unused.

}
/** Assigns the given data to a DOM element using monkey-patching */
export function attchContextDataToTarget(target: any, data: LViewData | Context) {

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

type attchContextDataToTarget => attachContextDataToTarget or maybe just attachLContext

* The internal view context which is specific to a given DOM element, directive or
* component instance
*/
export interface Context {

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

Currently the index points to Element, Component or directive. I don't think that is right. The Context should always be the same kind no matter where you retrieve it from.

The way to think about it is this: An element and a component have a one to one relationship to each other. (Not all elements have a component) So given a component the context should be the same as the context for its host element.

expect(getContext(component)).toEqual(getContext(getHostElement(component)));

NOTE: it does not have to be === just logically equivalent.

Here are the things which are associated with a DOM element:

  • DOM Element
  • Component (optional)
  • Zero or more directives.
  • Injector

This means that the Context should look more like this. (Also I think we should rename it to LContext to be more consistent with Ivy naming.)

interface LContext {
  lViewData: LVIewData;

  nativeIndex: number;
  native: RElement | null | undefined;

  componentIndex: number|undefined|null;
  component: {}|undefined|null;

  directiveIndexs: number[]|undefined|null;
  directives: Array<{}>;

  injectorIndex: number|undefined|null;
  injector: Injector|undefined|null;
}

NOTE: undefined means that we have not yet found the information. null means that we looked and there is no object.

As more methods such as getComponent or getInjector get called on the LContext more of the data gets filled in.

Show resolved Hide resolved packages/core/src/render3/context_discovery.ts Outdated
const index = findViaElement(lViewData !, element);
if (index >= 0) {
context = {index, native: element, lViewData};

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

extract to createLContext so that we are guaranteed that we have always the same hidden class for the object. Right now there is nothing to keep this and line 82 to have the same hidden class.

/** Locates the element within the given LViewData and returns the matching index */
function findViaElement(lViewData: LViewData, element: RElement): number {
for (let i = HEADER_OFFSET; i < lViewData.length; i++) {

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

lviewData.length is not right. It will search too much, we need to search subset. Check with @kara on how to do this properly.

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

You'll want to search only the def.consts section here, as there won't be nodes defined in the vars or hostVars sections. Alternatively, you can try walking the node tree directly with TNode.next and TNode.childIndex. That way, you're not searching pipes and local refs unnecessarily (which are consts).

This comment has been minimized.

@matsko

matsko Sep 5, 2018

Member

Not sure what you mean by def. If it's the component definition then how do you gain access to that?

This comment has been minimized.

@kara

kara Sep 5, 2018

Contributor

Yeah, the component def. You can get it through the view's host node. But I think the node tree is the way to go here because it skips pipes and refs.

// lives somewhere in the view data and the loop below will
// find it by comparing the component instance against each
// of the lElementNode instances that live in the lViewData
for (let i = HEADER_OFFSET; i < viewData.length; i++) {

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

same here. Start and end index of the array are way to bread unnecessarily. Check with @kara

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Rather than going through every item in the LViewData array, you could instead check viewData[TVIEW].components. It's a list of element indices for component hosts in that view and should be a much shorter list.

This comment has been minimized.

@matsko

matsko Sep 5, 2018

Member

This turns out to be empty (the components array) for an element that was bootstrapped? I had to stay with the existing implementation.

This comment has been minimized.

@kara

kara Sep 5, 2018

Contributor

Hmm, it shouldn't be empty if the directives have been matched. This array is how we process child components. Maybe you can show me tomorrow and we can figure out what's going on?

}
/** Locates the directive within the given LViewData and returns the matching index */
function findViaDirective(viewData: LViewData, directive: any): number {

This comment has been minimized.

@mhevery

mhevery Aug 31, 2018

Member

@kara can you verify that this is right.

export function getContext(target: any): Context|null {
let context = target[MONKEY_PATCH_KEY_NAME] as Context | LViewData | null;
if (context) {
// only when its an array is it considered a lViewData instance

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Typo: a lViewData -> an LViewData

/** Locates the component within the given LViewData and returns the matching index */
function findViaComponent(viewData: LViewData, component: any): number {
// if a component is monkey patched then it will (by default)
// have a referene to the lViewData of the parent view. The

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Typo: referene -> reference

// is actually present we can narrow down to which lElementNode
// contains the instance of the directive and then return the index
const directivesAcrossView = viewData[DIRECTIVES];
if (directivesAcrossView && directivesAcrossView.length) {

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

NIt: I don't think this length check is necessary, since the array is only created if it will have directives inside it.

if (directiveIndex >= 0) {
for (let i = HEADER_OFFSET; i < viewData.length; i++) {
const lNode = viewData[i] as LElementNode;
if (lNode && lNode.tNode) {

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Nit: All LNodes should have TNodes, so this check isn't necessary

This comment has been minimized.

@matsko

matsko Sep 5, 2018

Member

This check is here to avoid not getting an LNode (incase it's a pipe or something).

const directiveIndexStart = lNode.tNode.flags >> TNodeFlags.DirectiveStartingIndexShift;
const nextIndex = i + 1;
let nextNode = nextIndex < viewData.length ? viewData[nextIndex] : null;

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

The next index may not be the next node - it could be a pipe or a local ref (or a binding, since the loop checks the whole array). A more reliable way to find the next node is by walking the node tree like above. (I'd add a test with pipes/locals so we know we're handling this)

This comment has been minimized.

@matsko

matsko Sep 5, 2018

Member

Replaced with using the directive count.

// lives somewhere in the view data and the loop below will
// find it by comparing the component instance against each
// of the lElementNode instances that live in the lViewData
for (let i = HEADER_OFFSET; i < viewData.length; i++) {

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Rather than going through every item in the LViewData array, you could instead check viewData[TVIEW].components. It's a list of element indices for component hosts in that view and should be a much shorter list.

/** Locates the element within the given LViewData and returns the matching index */
function findViaElement(lViewData: LViewData, element: RElement): number {
for (let i = HEADER_OFFSET; i < lViewData.length; i++) {

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

You'll want to search only the def.consts section here, as there won't be nodes defined in the vars or hostVars sections. Alternatively, you can try walking the node tree directly with TNode.next and TNode.childIndex. That way, you're not searching pipes and local refs unnecessarily (which are consts).

directivesAcrossView.length;
// all that we care about is if the directive is in the list of
// directives assigned to the this element.

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Typo: "to the this" -> "to this"

nextNode = nextNode && nextNode.tNode ? nextNode : null;
const directiveIndexEnd = nextNode ?
(nextNode.tNode.flags >> TNodeFlags.DirectiveStartingIndexShift) :
directivesAcrossView.length;

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

This check doesn't look right to me. The next node may not have any directives, so you'll get 0 here and end up checking if the directive index is less than 0. Can we add a test for that case?

You can calculate the directiveIndexEnd for a node through TNodeFlags.DirectiveCountMask (add the count to the start).

This comment has been minimized.

@matsko

matsko Sep 5, 2018

Member

Replaced with using the directive count.

@@ -85,7 +86,7 @@ class Render3DebugContext implements DebugContext {
const currentNode = this.view[this.nodeIndex];
for (let dirIndex = 0; dirIndex < directives.length; dirIndex++) {
const directive = directives[dirIndex];
if (directive[NG_HOST_SYMBOL] === currentNode) {
if (getLElementNode(directive) === currentNode) {

This comment has been minimized.

@kara

kara Aug 31, 2018

Contributor

Yeah, there's an easier way to do this. The node's TNode has the range of directive indices, so you should only need to check TView.directives at that range to get the tokens.

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 5, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 5, 2018

@matsko

This comment has been minimized.

function getLNode(lViewData: LViewData, lElementIndex: number): LElementNode|null {
const value = lViewData[lElementIndex];
if (value) {
const lNodeValue = Array.isArray(value) ? value[StylingIndex.ElementPosition] : value;

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

This line looks like duplication of readElementValue from util. Any reason we can't re-use the code here?

const value = lViewData[lElementIndex];
if (value) {
const lNodeValue = Array.isArray(value) ? value[StylingIndex.ElementPosition] : value;
return lNodeValue && lNodeValue.tNode ? lNodeValue : null;

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

Why is the tNode check necessary? Shouldn't we always pass the correct index in? I think this can be removed (or seems like a comment / test is necessary)

if (componentIndices && componentIndices.length) {
for (let i = 0; i < componentIndices.length; i++) {
const elementComponentIndex = componentIndices[i];
if (lViewData[elementComponentIndex].data[CONTEXT] === componentInstance) {

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

We probably need to wrap this lookup in readElementValue in case the component has a styling context, no? Probably needs a test?

*/
function findViaComponent(lViewData: LViewData, componentInstance: {}): number {
const componentIndices = lViewData[TVIEW].components;
if (componentIndices && componentIndices.length) {

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

Nit: Length check isn't necessary. If it exists, it has length.

"name": "getDirectiveStartIndex"
},
{
"name": "getLElementNode"

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

Hmm I'm concerned with how much code this is bringing into "hello world". Is there any way we can break this function up into smaller pieces? See other comment on _getComponentHostLElementNode.

// (see `TNodeFlags` to see how the flag bit shifting
// values are used).
const count = lNode.tNode.flags & TNodeFlags.DirectiveCountMask;
return count ? (getDirectiveStartIndex(lNode) + count) : -1;

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

Nit: Seems like we should pass the start index as an arg to avoid calling the method twice unnecessarily.

@@ -2867,7 +2865,7 @@ function assertDataNext(index: number, arr?: any[]) {
export function _getComponentHostLElementNode<T>(component: T): LElementNode {
ngDevMode && assertDefined(component, 'expecting component got null');
const lElementNode = (component as any)[NG_HOST_SYMBOL] as LElementNode;
const lElementNode = getLElementNode(component) !;

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

This usage of getLElementNode brings in all of getContext and its helper functions (~12 fns). Since it's used in tickRootContext, this code will be present in most Angular apps (including "hello world"). I think we can reduce the amount of unnecessary code by breaking this function up.

  1. When _getComponentHostLElementNode is called in tickRootContext, we already know that we're passing in a component. We also already know the index of the component's node. It's 0 + the header offset. So we don't have to retain the code for checking whether it's a component or a directive or a native element and we shouldn't retain the code for searching the components array to find the right index. We should only need to get the LViewData instance and read the index. Maybe a new function like _getRootElementNode?

  2. When it's called in getRootView, we don't know the index of the LNode, but we do know we're passing in a component. In this case, we can probably drop all of the code to check directives and native elements.

I think we should be able to remove ~10 functions from "hello world" if we break up the code this way, and also ensure that we're not bringing directive matching code to the majority of apps that aren't using it.

node.tNode = tData[adjustedIndex] as TNode;
if (!tView.firstChild && type === TNodeType.Element) {

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

We probably want this to catch containers and element containers, since they both can contain directives. Otherwise, if you pass in a directive that's on one of those, it won't be found. Add a test?

tNode = tNode.parent.next || null;
} else {
tNode = null;
}

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

There's some duplication between lines 280-288 and lines 223 - 231. Can we move into a shared function?

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

@mhevery

mhevery approved these changes Sep 6, 2018

if (Array.isArray(lViewData)) {
applyPatch = true;
lNodeIndex =
isRootViewComponent ? HEADER_OFFSET : findViaComponent(lViewData !, componentInstance);

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

Can we split this up into two functions rather than having a boolean? We don't need findViaComponent in tickRootContext and the boolean keeps it from being tree-shaken.

We can call the root function in tickRootContext and the original one in getRootView (which does need to find the component). This should remove another 3 functions from "hello world".

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

If we don't need to eagerly patch contexts in change detection (since the context object hasn't been explicitly requested), we can also remove the context logic. We can create a small function for tickRootContext that just does this:

export function getRootElement(componentInstance: any) {
  const mp = readPatchedData(componentInstance);
  const lViewData = Array.isArray(mp) ? mp as LViewData : (mp as LContext).lViewData as LViewData;
  return readElementValue(lViewData[HEADER_OFFSET]) as LElementNode;
}

This would remove the following symbols from "hello world":

  • getComponentHostLElementNode
  • createLContext
  • findViaComponent
  • getLElementFromComponent
  • getRootView
}
const lNode = lNodeIndex >= 0 ? readElementValue(lViewData[lNodeIndex]) : null;
if (lNode && applyPatch) {

This comment has been minimized.

@kara

kara Sep 6, 2018

Contributor

It seems that this block can be moved up into the Array.isArray block if you also move up the read. Then you don't need the local for applyPatch and the logic is a bit more obvious / readable.

@mary-poppins

This comment has been minimized.

mary-poppins commented Sep 6, 2018

ngDevMode && assertDefined(component, 'expecting component got null');
const lElementNode = (component as any)[NG_HOST_SYMBOL] as LElementNode;
const lElementNode = isRootComponent ? getLElementFromRootComponent(component) ! :
getLElementFromComponent(component) !;

This comment has been minimized.

@kara

kara Sep 7, 2018

Contributor

This is still bringing in all the same symbols. The point of breaking it up is allowing everything in getLElementFromComponent to be tree-shaken away in tickRootContext.

@kara

kara approved these changes Sep 7, 2018

Discussed with @mhevery and @matsko and the remaining feedback will be implemented in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment