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

Spec style #22719

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/benchmarks/src/largetable/render3/table.ts
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ɵC as C, ɵE as E, ɵT as T, ɵV as V, ɵb as b, ɵcR as cR, ɵcr as cr, ɵdefineComponent as defineComponent, ɵdetectChanges as detectChanges, ɵe as e, ɵs as s, ɵt as t, ɵv as v} from '@angular/core';
import {ɵC as C, ɵE as E, ɵT as T, ɵV as V, ɵb as b, ɵcR as cR, ɵcr as cr, ɵdefineComponent as defineComponent, ɵdetectChanges as detectChanges, ɵe as e, ɵsn as sn, ɵt as t, ɵv as v} from '@angular/core';
import {ComponentDef} from '@angular/core/src/render3/interfaces/definition';

import {TableCell, buildTable, emptyTable} from '../util';
Expand Down Expand Up @@ -48,7 +48,7 @@ export class LargeTableComponent {
{ T(1); }
e();
}
s(0, 'background-color', b(cell.row % 2 ? '' : 'grey'));
sn(0, 'background-color', b(cell.row % 2 ? '' : 'grey'));
t(1, b(cell.value));
}
v();
Expand Down
6 changes: 3 additions & 3 deletions modules/benchmarks/src/tree/render3/tree.ts
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ɵC as C, ɵE as E, ɵT as T, ɵV as V, ɵb as b, ɵcR as cR, ɵcr as cr, ɵdefineComponent as defineComponent, ɵdetectChanges as _detectChanges, ɵe as e, ɵi1 as i1, ɵp as p, ɵs as s, ɵt as t, ɵv as v} from '@angular/core';
import {ɵC as C, ɵE as E, ɵT as T, ɵV as V, ɵb as b, ɵcR as cR, ɵcr as cr, ɵdefineComponent as defineComponent, ɵdetectChanges as _detectChanges, ɵe as e, ɵi1 as i1, ɵp as p, ɵsn as sn, ɵt as t, ɵv as v} from '@angular/core';
import {ComponentDef} from '@angular/core/src/render3/interfaces/definition';

import {TreeNode, buildTree, emptyTree} from '../util';
Expand Down Expand Up @@ -46,7 +46,7 @@ export class TreeComponent {
C(2);
C(3);
}
s(0, 'background-color', b(ctx.data.depth % 2 ? '' : 'grey'));
sn(0, 'background-color', b(ctx.data.depth % 2 ? '' : 'grey'));
t(1, i1(' ', ctx.data.value, ' '));
cR(2);
{
Expand Down Expand Up @@ -114,7 +114,7 @@ export function TreeTpl(ctx: TreeNode, cm: boolean) {
}
e();
}
s(1, 'background-color', b(ctx.depth % 2 ? '' : 'grey'));
sn(1, 'background-color', b(ctx.depth % 2 ? '' : 'grey'));
t(2, i1(' ', ctx.value, ' '));
cR(3);
{
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler/src/render3/r3_identifiers.ts
Expand Up @@ -26,9 +26,9 @@ export class Identifiers {

static elementAttribute: o.ExternalReference = {name: 'ɵa', moduleName: CORE};

static elementClass: o.ExternalReference = {name: 'ɵk', moduleName: CORE};
static elementClassNamed: o.ExternalReference = {name: 'ɵkn', moduleName: CORE};

static elementStyle: o.ExternalReference = {name: 'ɵs', moduleName: CORE};
static elementStyleNamed: o.ExternalReference = {name: 'ɵsn', moduleName: CORE};

static containerCreate: o.ExternalReference = {name: 'ɵC', moduleName: CORE};

Expand Down
4 changes: 2 additions & 2 deletions packages/compiler/src/render3/r3_view_compiler.ts
Expand Up @@ -210,8 +210,8 @@ function unsupported(feature: string): never {
const BINDING_INSTRUCTION_MAP: {[index: number]: o.ExternalReference | undefined} = {
[PropertyBindingType.Property]: R3.elementProperty,
[PropertyBindingType.Attribute]: R3.elementAttribute,
[PropertyBindingType.Class]: R3.elementClass,
[PropertyBindingType.Style]: R3.elementStyle
[PropertyBindingType.Class]: R3.elementClassNamed,
[PropertyBindingType.Style]: R3.elementStyleNamed
};

function interpolate(args: o.Expression[]): o.Expression {
Expand Down
4 changes: 3 additions & 1 deletion packages/core/src/core_render3_private_export.ts
Expand Up @@ -63,8 +63,10 @@ export {
p as ɵp,
pD as ɵpD,
a as ɵa,
k as ɵk,
s as ɵs,
sn as ɵsn,
k as ɵk,
kn as ɵkn,
t as ɵt,
v as ɵv,
st as ɵst,
Expand Down
13 changes: 12 additions & 1 deletion packages/core/src/render3/PERF_NOTES.md
Expand Up @@ -50,4 +50,15 @@ export function getExported() { return exported; }
export function setExported(v) { exported = v; }
```

Also writing to a property of `exports` might change its hidden class resulting in megamorphic access.
Also writing to a property of `exports` might change its hidden class resulting in megamorphic access.

## Iterating over Keys of an Object.

https://jsperf.com/object-keys-vs-for-in-with-closure/3 implies that `Object.keys` is the fastest way of iterating
over properties of an object.

```
for (var i = 0, keys = Object.keys(obj); i < keys.length; i++) {
const key = keys[i];
}
```
3 changes: 3 additions & 0 deletions packages/core/src/render3/index.ts
Expand Up @@ -46,10 +46,12 @@ export {

elementAttribute as a,
elementClass as k,
elementClassNamed as kn,
elementEnd as e,
elementProperty as p,
elementStart as E,
elementStyle as s,
elementStyleNamed as sn,

listener as L,
store as st,
Expand All @@ -65,6 +67,7 @@ export {
embeddedViewEnd as v,
detectChanges,
markDirty,
tick,
} from './instructions';

export {
Expand Down
73 changes: 65 additions & 8 deletions packages/core/src/render3/instructions.ts
Expand Up @@ -866,7 +866,7 @@ function generatePropertyAliases(lNodeFlags: number, direction: BindingDirection
}

/**
* Add or remove a class in a classList.
* Add or remove a class in a `classList` on a DOM element.
*
* This instruction is meant to handle the [class.foo]="exp" case
*
Expand All @@ -875,7 +875,7 @@ function generatePropertyAliases(lNodeFlags: number, direction: BindingDirection
* renaming as part of minification.
* @param value A value indicating if a given class should be added or removed.
*/
export function elementClass<T>(index: number, className: string, value: T | NO_CHANGE): void {
export function elementClassNamed<T>(index: number, className: string, value: T | NO_CHANGE): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

the elementClass variant seems to be the most specific / counter-intuitive. Keep elementClass() for this one and find a meaningful name for the other one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whatever name we come up with it should be consistent with style. I am open for new name but it should be consistent.

if (value !== NO_CHANGE) {
const lElement = data[index] as LElementNode;
if (value) {
Expand All @@ -889,6 +889,29 @@ export function elementClass<T>(index: number, className: string, value: T | NO_
}
}

/**
* Set the `className` property on a DOM element.
*
* This instruction is meant to handle the `[class]="exp"` usage.
*
* `elementClass` instruction writes the value to the "element's" `className` property.
*
* @param index The index of the element to update in the data array
* @param value A value indicating a set of classes which should be applied. The method overrides
* any existing classes. The value is stringified (`toString`) before it is applied to the
* element.
*/
export function elementClass<T>(index: number, value: T | NO_CHANGE): void {
if (value !== NO_CHANGE) {
// TODO: This is a naive implementation which simply writes value to the `className`. In the
// future
// we will add logic here which would work with the animation code.
const lElement: LElementNode = data[index];
isProceduralRenderer(renderer) ? renderer.setProperty(lElement.native, 'className', value) :
lElement.native['className'] = stringify(value);
}
}

/**
* Update a given style on an Element.
*
Expand All @@ -900,31 +923,65 @@ export function elementClass<T>(index: number, className: string, value: T | NO_
* @param sanitizer An optional function used to transform the value typically used for
* sanitization.
*/
export function elementStyle<T>(
export function elementStyleNamed<T>(
index: number, styleName: string, value: T | NO_CHANGE, suffix?: string): void;
export function elementStyle<T>(
export function elementStyleNamed<T>(
index: number, styleName: string, value: T | NO_CHANGE, sanitizer?: Sanitizer): void;
export function elementStyle<T>(
export function elementStyleNamed<T>(
index: number, styleName: string, value: T | NO_CHANGE,
suffixOrSanitizer?: string | Sanitizer): void {
if (value !== NO_CHANGE) {
const lElement = data[index] as LElementNode;
const lElement: LElementNode = data[index];
if (value == null) {
isProceduralRenderer(renderer) ?
renderer.removeStyle(lElement.native, styleName, RendererStyleFlags3.DashCase) :
lElement.native.style.removeProperty(styleName);
lElement.native['style'].removeProperty(styleName);
} else {
let strValue =
typeof suffixOrSanitizer == 'function' ? suffixOrSanitizer(value) : stringify(value);
if (typeof suffixOrSanitizer == 'string') strValue = strValue + suffixOrSanitizer;
isProceduralRenderer(renderer) ?
renderer.setStyle(lElement.native, styleName, strValue, RendererStyleFlags3.DashCase) :
lElement.native.style.setProperty(styleName, strValue);
lElement.native['style'].setProperty(styleName, strValue);
}
}
}

/**
* Set the `style` property on a DOM element.
*
* This instruction is meant to handle the `[style]="exp"` usage.
*
*
* @param index The index of the element to update in the data array
* @param value A value indicating if a given style should be added or removed.
* The expected shape of `value` is an object where keys are style names and the values
* are their corresponding values to set. If value is falsy than the style is remove. An absence
* of style does not cause that style to be removed. `NO_CHANGE` implies that no update should be
* performed.
*/
export function elementStyle<T>(
Copy link
Contributor

Choose a reason for hiding this comment

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

remove <T> ?

Copy link
Contributor Author

@mhevery mhevery Mar 13, 2018

Choose a reason for hiding this comment

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

Per comment above I think we should keep the . Unless you feel strongly

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it apply with {[styleName: string]: any} | NO_CHANGE ?

index: number, value: {[styleName: string]: any} | NO_CHANGE): void {
if (value !== NO_CHANGE) {
// TODO: This is a naive implementation which simply writes value to the `style`. In the future
// we will add logic here which would work with the animation code.
const lElement = data[index] as LElementNode;
if (isProceduralRenderer(renderer)) {
renderer.setProperty(lElement.native, 'style', value);
} else {
const style = lElement.native['style'];
for (let i = 0, keys = Object.keys(value); i < keys.length; i++) {
const styleName: string = keys[i];
const styleValue: any = (value as any)[styleName];
Copy link
Contributor

Choose a reason for hiding this comment

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

drop ( as any) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't it is an error.

Copy link
Contributor

@vicb vicb Mar 13, 2018

Choose a reason for hiding this comment

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

Still true with {[styleName: string]: any} | NO_CHANGE ?

edit: oh nevermind this one. Must be because NO_CHANGE type vs NO_CHANGE value.

styleValue == null ? style.removeProperty(styleName) :
style.setProperty(styleName, styleValue);
}
}
}
}



//////////////////////////
//// Text
//////////////////////////
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/render3/interfaces/renderer.ts
Expand Up @@ -121,6 +121,7 @@ export interface RNode {
export interface RElement extends RNode {
style: RCssStyleDeclaration;
classList: RDomTokenList;
className: string;
setAttribute(name: string, value: string): void;
removeAttribute(name: string): void;
setAttributeNS(namespaceURI: string, qualifiedName: string, value: string): void;
Expand Down
41 changes: 36 additions & 5 deletions packages/core/test/render3/compiler_canonical/elements_spec.ts
Expand Up @@ -9,7 +9,7 @@
import {browserDetection} from '@angular/platform-browser/testing/src/browser_util';
import {ChangeDetectionStrategy, ChangeDetectorRef, Component, ContentChild, ContentChildren, Directive, HostBinding, HostListener, Injectable, Input, NgModule, OnDestroy, Optional, Pipe, PipeTransform, QueryList, SimpleChanges, TemplateRef, ViewChild, ViewChildren, ViewContainerRef} from '../../../src/core';
import * as $r3$ from '../../../src/core_render3_private_export';
import {renderComponent, toHtml} from '../render_util';
import {ComponentFixture, renderComponent, toHtml} from '../render_util';

/// See: `normative.md`
describe('elements', () => {
Expand Down Expand Up @@ -168,7 +168,7 @@ describe('elements', () => {
$r3$.ɵE(0, 'div');
$r3$.ɵe();
}
$r3$.ɵk(0, 'foo', $r3$.ɵb(ctx.someFlag));
$r3$.ɵkn(0, 'foo', $r3$.ɵb(ctx.someFlag));
}
});
// /NORMATIVE
Expand Down Expand Up @@ -202,8 +202,8 @@ describe('elements', () => {
$r3$.ɵE(0, 'div');
$r3$.ɵe();
}
$r3$.ɵs(0, 'color', $r3$.ɵb(ctx.someColor));
$r3$.ɵs(0, 'width', $r3$.ɵb(ctx.someWidth), 'px');
$r3$.ɵsn(0, 'color', $r3$.ɵb(ctx.someColor));
$r3$.ɵsn(0, 'width', $r3$.ɵb(ctx.someWidth), 'px');
}
});
// /NORMATIVE
Expand Down Expand Up @@ -251,7 +251,7 @@ describe('elements', () => {
$r3$.ɵe();
}
$r3$.ɵp(0, 'id', $r3$.ɵb(ctx.someString + 1));
$r3$.ɵk(0, 'foo', $r3$.ɵb(ctx.someString == 'initial'));
$r3$.ɵkn(0, 'foo', $r3$.ɵb(ctx.someString == 'initial'));
}
});
// /NORMATIVE
Expand All @@ -264,5 +264,36 @@ describe('elements', () => {
$r3$.ɵdetectChanges(comp);
expect(toHtml(comp)).toEqual('<div class="" id="changed1" style="color: red;"></div>');
});

it('should bind [class] and [style] to the element', () => {
type $StyleComponent$ = StyleComponent;

@Component(
{selector: 'style-comp', template: `<div [class]="classExp" [style]="styleExp"></div>`})
class StyleComponent {
classExp: string[]|string = 'some-name';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to support {[name: string]: boolean} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, not sure yet...

styleExp: {[name: string]: string} = {'background-color': 'red'};

// NORMATIVE
static ngComponentDef = $r3$.ɵdefineComponent({
type: StyleComponent,
tag: 'style-comp',
factory: function StyleComponent_Factory() { return new StyleComponent(); },
template: function StyleComponent_Template(ctx: $StyleComponent$, cm: $boolean$) {
if (cm) {
$r3$.ɵE(0, 'div');
$r3$.ɵe();
}
$r3$.ɵk(0, $r3$.ɵb(ctx.classExp));
$r3$.ɵs(0, $r3$.ɵb(ctx.styleExp));
}
});
// /NORMATIVE
}

const styleFixture = new ComponentFixture(StyleComponent);
expect(styleFixture.html)
.toEqual(`<div class="some-name" style="background-color: red;"></div>`);
});
});
});
Expand Up @@ -49,7 +49,7 @@ describe('compiler sanitization', () => {
}
$r3$.ɵp(0, 'innerHTML', $r3$.ɵb(ctx.innerHTML), $r3$.ɵsanitizeHtml);
$r3$.ɵp(0, 'hidden', $r3$.ɵb(ctx.hidden));
$r3$.ɵs(1, 'background-image', $r3$.ɵb(ctx.style), $r3$.ɵsanitizeStyle);
$r3$.ɵsn(1, 'background-image', $r3$.ɵb(ctx.style), $r3$.ɵsanitizeStyle);
$r3$.ɵp(1, 'src', $r3$.ɵb(ctx.url), $r3$.ɵsanitizeUrl);
$r3$.ɵa(1, 'srcset', $r3$.ɵb(ctx.url), $r3$.ɵsanitizeUrl);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/render3/exports_spec.ts
Expand Up @@ -7,7 +7,7 @@
*/

import {defineComponent, defineDirective} from '../../src/render3/index';
import {bind, container, containerRefreshEnd, containerRefreshStart, elementAttribute, elementClass, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, load, text, textBinding} from '../../src/render3/instructions';
import {bind, container, containerRefreshEnd, containerRefreshStart, elementAttribute, elementClassNamed, elementEnd, elementProperty, elementStart, embeddedViewEnd, embeddedViewStart, load, text, textBinding} from '../../src/render3/instructions';

import {renderToHtml} from './render_util';

Expand Down Expand Up @@ -169,7 +169,7 @@ describe('exports', () => {
elementEnd();
}
let myInput = elementStart(1);
elementClass(0, 'red', bind(myInput && (myInput as any).checked));
elementClassNamed(0, 'red', bind(myInput && (myInput as any).checked));
}

expect(renderToHtml(Template, {}))
Expand Down