cargdev/angular
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(ivy): remove def.attributes in favor of the elementHostAttrs instruction (#28089)

Browse Source 
Up until this point, all static attribute values (things like `title` and `id`)
defined within the `host` are of a Component/Directive definition were
generated into a `def.attributes` array and then processed at runtime.
This design decision does not lend itself well to tree-shaking and is
inconsistent with other static values such as styles and classes.

This fix ensures that all static attribute values (attributes, classes,
and styles) that exist within a host definition for components and
directives are all assigned via the `elementHostAttrs` instruction.

```
// before
defineDirective({
  ...
  attributes: ['title', 'my title']
  ...
})

//now
defineDirective({
  ...
  hostBindings: function() {
    if (create) {
      elementHostAttrs(..., ['title', 'my-title']);
    }
    ...
  }
  ...
})
```

PR Close #28089
This commit is contained in:
Matias Niemelä
2019-01-11 14:03:37 -08:00
committed by Andrew Kushnir
parent e62eeed7d4
commit 693045165c
14 changed files with 226 additions and 127 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
packages/core/src/render3/definition.ts
View File

@ -68,14 +68,6 @@ export function defineComponent<T>(componentDefinition: {
*/
vars: number;
/**
* Static attributes to set on host element.
*
* Even indices: attribute name
* Odd indices: attribute value
*/
attributes?: string[];
/**
* A map of input names.
*
@ -260,7 +252,6 @@ export function defineComponent<T>(componentDefinition: {
hostBindings: componentDefinition.hostBindings || null,
contentQueries: componentDefinition.contentQueries || null,
contentQueriesRefresh: componentDefinition.contentQueriesRefresh || null,
attributes: componentDefinition.attributes || null,
declaredInputs: declaredInputs,
inputs: null !, // assigned in noSideEffects
outputs: null !, // assigned in noSideEffects
@ -516,14 +507,6 @@ export const defineDirective = defineComponent as any as<T>(directiveDefinition:
*/
factory: (t: Type<T>| null) => T;
/**
* Static attributes to set on host element.
*
* Even indices: attribute name
* Odd indices: attribute value
*/
attributes?: string[];
/**
* A map of input names.
*

122
packages/core/src/render3/instructions.ts
View File

@ -752,31 +752,62 @@ function createViewBlueprint(bindingStartIndex: number, initialViewLength: numbe
return blueprint;
}
function setUpAttributes(native: RElement, attrs: TAttributes): void {
/**
* Assigns all attribute values to the provided element via the inferred renderer.
*
* This function accepts two forms of attribute entries:
*
* default: (key, value):
* attrs = [key1, value1, key2, value2]
*
* namespaced: (NAMESPACE_MARKER, uri, name, value)
* attrs = [NAMESPACE_MARKER, uri, name, value, NAMESPACE_MARKER, uri, name, value]
*
* The `attrs` array can contain a mix of both the default and namespaced entries.
* The "default" values are set without a marker, but if the function comes across
* a marker value then it will attempt to set a namespaced value. If the marker is
* not of a namespaced value then the function will quit and return the index value
* where it stopped during the iteration of the attrs array.
*
* See [AttributeMarker] to understand what the namespace marker value is.
*
* Note that this instruction does not support assigning style and class values to
* an element. See `elementStart` and `elementHostAttrs` to learn how styling values
* are applied to an element.
*
* @param native The element that the attributes will be assigned to
* @param attrs The attribute array of values that will be assigned to the element
* @returns the index value that was last accessed in the attributes array
*/
function setUpAttributes(native: RElement, attrs: TAttributes): number {
const renderer = getLView()[RENDERER];
const isProc = isProceduralRenderer(renderer);
let i = 0;
let i = 0;
while (i < attrs.length) {
const attrName = attrs[i++];
if (typeof attrName == 'number') {
if (attrName === AttributeMarker.NamespaceURI) {
// Namespaced attributes
const namespaceURI = attrs[i++] as string;
const attrName = attrs[i++] as string;
const attrVal = attrs[i++] as string;
ngDevMode && ngDevMode.rendererSetAttribute++;
isProc ?
(renderer as ProceduralRenderer3)
.setAttribute(native, attrName, attrVal, namespaceURI) :
native.setAttributeNS(namespaceURI, attrName, attrVal);
} else {
// All other `AttributeMarker`s are ignored here.
const value = attrs[i];
if (typeof value === 'number') {
// only namespaces are supported. Other value types (such as style/class
// entries) are not supported in this function.
if (value !== AttributeMarker.NamespaceURI) {
break;
}
// we just landed on the marker value ... therefore
// we should skip to the next entry
i++;
const namespaceURI = attrs[i++] as string;
const attrName = attrs[i++] as string;
const attrVal = attrs[i++] as string;
ngDevMode && ngDevMode.rendererSetAttribute++;
isProc ?
(renderer as ProceduralRenderer3).setAttribute(native, attrName, attrVal, namespaceURI) :
native.setAttributeNS(namespaceURI, attrName, attrVal);
} else {
/// attrName is string;
const attrVal = attrs[i++];
const attrName = value as string;
const attrVal = attrs[++i];
if (attrName !== NG_PROJECT_AS_ATTR_NAME) {
// Standard attributes
ngDevMode && ngDevMode.rendererSetAttribute++;
@ -791,8 +822,15 @@ function setUpAttributes(native: RElement, attrs: TAttributes): void {
native.setAttribute(attrName as string, attrVal as string);
}
}
i++;
}
}
// another piece of code may iterate over the same attributes array. Therefore
// it may be helpful to return the exact spot where the attributes array exited
// whether by running into an unsupported marker or if all the static values were
// iterated over.
return i;
}
export function createError(text: string, token: any) {
@ -1257,19 +1295,41 @@ export function elementStyling(
}
/**
* Assign static styling values to a host element.
* Assign static attribute values to a host element.
*
* This instruction will assign static attribute values as well as class and style
* values to an element within the host bindings function. Since attribute values
* can consist of different types of values, the `attrs` array must include the values in
* the following format:
*
* attrs = [
* // static attributes (like `title`, `name`, `id`...)
* attr1, value1, attr2, value,
*
* // a single namespace value (like `x:id`)
* NAMESPACE_MARKER, namespaceUri1, name1, value1,
*
* // another single namespace value (like `x:name`)
* NAMESPACE_MARKER, namespaceUri2, name2, value2,
*
* // a series of CSS classes that will be applied to the element (no spaces)
* CLASSES_MARKER, class1, class2, class3,
*
* // a series of CSS styles (property + value) that will be applied to the element
* STYLES_MARKER, prop1, value1, prop2, value2
* ]
*
* All non-class and non-style attributes must be defined at the start of the list
* first before all class and style values are set. When there is a change in value
* type (like when classes and styles are introduced) a marker must be used to separate
* the entries. The marker values themselves are set via entries found in the
* [AttributeMarker] enum.
*
* NOTE: This instruction is meant to used from `hostBindings` function only.
*
* @param directive A directive instance the styling is associated with.
* @param attrs An array containing class and styling information. The values must be marked with
* `AttributeMarker`.
*
* ```
* var attrs = [AttributeMarker.Classes, 'foo', 'bar',
* AttributeMarker.Styles, 'width', '100px', 'height, '200px']
* elementHostAttrs(directive, attrs);
* ```
* @param attrs An array of static values (attributes, classes and styles) with the correct marker
* values.
*
* @publicApi
*/
@ -1278,7 +1338,10 @@ export function elementHostAttrs(directive: any, attrs: TAttributes) {
if (!tNode.stylingTemplate) {
tNode.stylingTemplate = initializeStaticStylingContext(attrs);
}
patchContextWithStaticAttrs(tNode.stylingTemplate, attrs, directive);
const lView = getLView();
const native = getNativeByTNode(tNode, lView) as RElement;
const i = setUpAttributes(native, attrs);
patchContextWithStaticAttrs(tNode.stylingTemplate, attrs, i, directive);
}
/**
@ -1658,11 +1721,6 @@ function postProcessBaseDirective<T>(
if (native) {
attachPatchData(native, lView);
}
// TODO(misko): setUpAttributes should be a feature for better treeshakability.
if (def.attributes != null && previousOrParentTNode.type == TNodeType.Element) {
setUpAttributes(native as RElement, def.attributes as string[]);
}
}

8
packages/core/src/render3/interfaces/definition.ts
View File

@ -140,14 +140,6 @@ export interface DirectiveDef<T> extends BaseDef<T> {
/** Refreshes host bindings on the associated directive. */
hostBindings: HostBindingsFunction<T>|null;
/**
* Static attributes to set on host element.
*
* Even indices: attribute name
* Odd indices: attribute value
*/
readonly attributes: string[]|null;
/* The following are lifecycle hooks for this component */
onInit: (() => void)|null;
doCheck: (() => void)|null;

4
packages/core/src/render3/styling/class_and_style_bindings.ts
View File

@ -75,7 +75,7 @@ export function initializeStaticContext(attrs: TAttributes) {
* @param directive the directive instance with which static data is associated with.
*/
export function patchContextWithStaticAttrs(
context: StylingContext, attrs: TAttributes, directive: any): void {
context: StylingContext, attrs: TAttributes, startingIndex: number, directive: any): void {
// If the styling context has already been patched with the given directive's bindings,
// then there is no point in doing it again. The reason why this may happen (the directive
// styling being patched twice) is because the `stylingBinding` function is called each time
@ -89,7 +89,7 @@ export function patchContextWithStaticAttrs(
let initialStyles: InitialStylingValues|null = null;
let mode = -1;
for (let i = 0; i < attrs.length; i++) {
for (let i = startingIndex; i < attrs.length; i++) {
const attr = attrs[i];
if (typeof attr == 'number') {
mode = attr;

12
packages/core/test/bundling/hello_world/bundle.golden_symbols.json
View File

@ -2,9 +2,6 @@
{
"name": "ACTIVE_INDEX"
},
{
"name": "ANIMATION_PROP_PREFIX"
},
{
"name": "BINDING_INDEX"
},
@ -77,9 +74,6 @@
{
"name": "NG_PIPE_DEF"
},
{
"name": "NG_PROJECT_AS_ATTR_NAME"
},
{
"name": "NO_CHANGE"
},
@ -320,9 +314,6 @@
{
"name": "invertObject"
},
{
"name": "isAnimationProp"
},
{
"name": "isComponentDef"
},
@ -434,9 +425,6 @@
{
"name": "setTNodeAndViewData"
},
{
"name": "setUpAttributes"
},
{
"name": "syncViewWithBlueprint"
},

6
packages/core/test/render3/host_binding_spec.ts
View File

@ -965,7 +965,11 @@ describe('host bindings', () => {
selectors: [['', 'hostAttributeDir', '']],
type: HostAttributeDir,
factory: () => new HostAttributeDir(),
attributes: ['role', 'listbox']
hostBindings: function(rf, ctx, elIndex) {
if (rf & RenderFlags.Create) {
elementHostAttrs(ctx, ['role', 'listbox']);
}
}
});
}

11
packages/core/test/render3/integration_spec.ts
View File

@ -478,8 +478,12 @@ describe('render3 integration test', () => {
factory: () => new HostAttributeComp(),
consts: 0,
vars: 0,
hostBindings: function(rf, ctx, elIndex) {
if (rf & RenderFlags.Create) {
elementHostAttrs(ctx, ['role', 'button']);
}
},
template: (rf: RenderFlags, ctx: HostAttributeComp) => {},
attributes: ['role', 'button']
});
}
@ -1706,8 +1710,8 @@ describe('render3 integration test', () => {
rf: RenderFlags, ctx: DirWithInitialStyling, elementIndex: number) {
if (rf & RenderFlags.Create) {
elementHostAttrs(ctx, [
AttributeMarker.Classes, 'heavy', 'golden', AttributeMarker.Styles, 'color',
'purple', 'font-weight', 'bold'
'title', 'foo', AttributeMarker.Classes, 'heavy', 'golden',
AttributeMarker.Styles, 'color', 'purple', 'font-weight', 'bold'
]);
}
}
@ -1735,6 +1739,7 @@ describe('render3 integration test', () => {
const classes = target.getAttribute('class') !.split(/\s+/).sort();
expect(classes).toEqual(['big', 'golden', 'heavy']);
expect(target.getAttribute('title')).toEqual('foo');
expect(target.style.getPropertyValue('color')).toEqual('black');
expect(target.style.getPropertyValue('font-size')).toEqual('200px');
expect(target.style.getPropertyValue('font-weight')).toEqual('bold');

4
packages/core/test/render3/styling/class_and_style_bindings_spec.ts
View File

@ -70,7 +70,7 @@ describe('style and class based bindings', () => {
attrs.push(AttributeMarker.Styles);
attrs.push(...styles);
}
patchContextWithStaticAttrs(context, attrs, directiveRef || null);
patchContextWithStaticAttrs(context, attrs, 0, directiveRef || null);
}
function getRootContextInternal(lView: LView) { return lView[CONTEXT] as RootContext; }
@ -1388,7 +1388,7 @@ describe('style and class based bindings', () => {
let styles: any = {fontSize: ''};
updateStyleProp(stylingContext, 0, '');
updateStylingMap(stylingContext, null, styles);
patchContextWithStaticAttrs(stylingContext, [], otherDirective);
patchContextWithStaticAttrs(stylingContext, [], 0, otherDirective);
getStyles(stylingContext, otherDirective);
expect(store.getValues()).toEqual({});

4
packages/core/test/render3/view_container_ref_spec.ts
View File

@ -10,7 +10,7 @@ import {ChangeDetectorRef, Component as _Component, ComponentFactoryResolver, El
import {ViewEncapsulation} from '../../src/metadata';
import {AttributeMarker, defineComponent, defineDirective, definePipe, injectComponentFactoryResolver, load, query, queryRefresh} from '../../src/render3/index';
import {allocHostVars, bind, container, containerRefreshEnd, containerRefreshStart, directiveInject, element, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation3, nextContext, projection, projectionDef, reference, template, text, textBinding} from '../../src/render3/instructions';
import {allocHostVars, bind, container, containerRefreshEnd, containerRefreshStart, directiveInject, element, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, interpolation1, interpolation3, nextContext, projection, projectionDef, reference, template, text, textBinding, elementHostAttrs} from '../../src/render3/instructions';
import {RenderFlags} from '../../src/render3/interfaces/definition';
import {RElement} from '../../src/render3/interfaces/renderer';
import {templateRefExtractor} from '../../src/render3/view_engine_compatibility_prebound';
@ -1855,9 +1855,9 @@ describe('ViewContainerRef', () => {
consts: 0,
vars: 0,
template: (rf: RenderFlags, cmp: HostBindingCmpt) => {},
attributes: ['id', 'attribute'],
hostBindings: function(rf: RenderFlags, ctx: HostBindingCmpt, elIndex: number) {
if (rf & RenderFlags.Create) {
elementHostAttrs(ctx, ['id', 'attribute']);
allocHostVars(1);
}
if (rf & RenderFlags.Update) {