refactor(compiler): distinguish two-way bindings in the AST by crisbeto · Pull Request #54065 · angular/angular · GitHub
Skip to content

refactor(compiler): distinguish two-way bindings in the AST #54065

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.

Sign up for GitHub

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

Closed
crisbeto wants to merge 1 commit into from
Closed

refactor(compiler): distinguish two-way bindings in the AST #54065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View file
Open in desktop
7 changes: 6 additions & 1 deletion packages/compiler/src/expression_parser/ast.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,14 +842,17 @@ export class ParsedProperty {
export enum ParsedPropertyType {
DEFAULT,
LITERAL_ATTR,
ANIMATION
ANIMATION,
TWO_WAY,
}

export const enum ParsedEventType {
// DOM or Directive event
Regular,
// Animation specific event
Animation,
// Event side of a two-way binding (e.g. `[(property)]="expression"`).
TwoWay,
}

export class ParsedEvent {
Expand Down Expand Up @@ -882,6 +885,8 @@ export const enum BindingType {
Style,
// A binding to an animation reference (e.g. `[animate.key]="expression"`).
Animation,
// Property side of a two-way binding (e.g. `[(property)]="expression"`).
TwoWay,
}

export class BoundElementProperty {
Expand Down
8 changes: 4 additions & 4 deletions packages/compiler/src/render3/r3_template_transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ class HtmlAstToIvyAst implements html.Visitor {
const identifier = bindParts[IDENT_KW_IDX];
const keySpan = createKeySpan(srcSpan, bindParts[KW_BIND_IDX], identifier);
this.bindingParser.parsePropertyBinding(
identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
identifier, value, false, false, srcSpan, absoluteOffset, attribute.valueSpan,
matchableAttributes, parsedProperties, keySpan);

} else if (bindParts[KW_LET_IDX]) {
Expand Down Expand Up @@ -502,7 +502,7 @@ class HtmlAstToIvyAst implements html.Visitor {
const identifier = bindParts[IDENT_KW_IDX];
const keySpan = createKeySpan(srcSpan, bindParts[KW_BINDON_IDX], identifier);
this.bindingParser.parsePropertyBinding(
identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
identifier, value, false, true, srcSpan, absoluteOffset, attribute.valueSpan,
matchableAttributes, parsedProperties, keySpan);
this.parseAssignmentEvent(
identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents,
Expand Down Expand Up @@ -536,14 +536,14 @@ class HtmlAstToIvyAst implements html.Visitor {
const keySpan = createKeySpan(srcSpan, delims.start, identifier);
if (delims.start === BINDING_DELIMS.BANANA_BOX.start) {
this.bindingParser.parsePropertyBinding(
identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
identifier, value, false, true, srcSpan, absoluteOffset, attribute.valueSpan,
matchableAttributes, parsedProperties, keySpan);
this.parseAssignmentEvent(
identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents,
keySpan);
} else if (delims.start === BINDING_DELIMS.PROPERTY.start) {
this.bindingParser.parsePropertyBinding(
identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
identifier, value, false, false, srcSpan, absoluteOffset, attribute.valueSpan,
matchableAttributes, parsedProperties, keySpan);
} else {
const events: ParsedEvent[] = [];
Expand Down
5 changes: 3 additions & 2 deletions packages/compiler/src/render3/view/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,8 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
element.inputs.forEach(input => {
const stylingInputWasSet = stylingBuilder.registerBoundInput(input);
if (!stylingInputWasSet) {
if (input.type === BindingType.Property && input.i18n) {
if ((input.type === BindingType.Property || input.type === BindingType.TwoWay) &&
input.i18n) {
boundI18nAttrs.push(input);
} else {
allOtherInputs.push(input);
Expand Down Expand Up @@ -865,7 +866,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
}
this.allocateBindingSlots(value);

if (inputType === BindingType.Property) {
if (inputType === BindingType.Property || inputType === BindingType.TwoWay) {
if (value instanceof Interpolation) {
// prop="{{value}}" and friends
this.interpolatedUpdateInstruction(
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler/src/render3/view/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ function getAttrsForDirectiveMatching(elOrTpl: t.Element|t.Template): {[name: st
});

elOrTpl.inputs.forEach(i => {
if (i.type === BindingType.Property) {
if (i.type === BindingType.Property || i.type === BindingType.TwoWay) {
attributesMap[i.name] = '';
}
});
Expand Down
10 changes: 6 additions & 4 deletions packages/compiler/src/template/pipeline/src/ingest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ export function ingestHostAttribute(
}

export function ingestHostEvent(job: HostBindingCompilationJob, event: e.ParsedEvent) {
const [phase, target] = event.type === e.ParsedEventType.Regular ? [null, event.targetOrPhase] :
[event.targetOrPhase, null];
const [phase, target] = event.type !== e.ParsedEventType.Animation ? [null, event.targetOrPhase] :
[event.targetOrPhase, null];
const eventBinding = ir.createListenerOp(
job.root.xref, new ir.SlotHandle(), event.name, null,
makeListenerHandlerOps(job.root, event.handler, event.handlerSpan), phase, target, true,
Expand Down Expand Up @@ -846,6 +846,8 @@ function convertAstWithInterpolation(
// TODO: Can we populate Template binding kinds in ingest?
const BINDING_KINDS = new Map<e.BindingType, ir.BindingKind>([
[e.BindingType.Property, ir.BindingKind.Property],
// TODO(crisbeto): we'll need a different BindingKind for two-way bindings.
[e.BindingType.TwoWay, ir.BindingKind.Property],
[e.BindingType.Attribute, ir.BindingKind.Attribute],
[e.BindingType.Class, ir.BindingKind.ClassName],
[e.BindingType.Style, ir.BindingKind.StyleProperty],
Expand Down Expand Up @@ -1042,8 +1044,8 @@ function createTemplateBinding(
// update instruction.
if (templateKind === ir.TemplateKind.Structural) {
if (!isStructuralTemplateAttribute &&
(type === e.BindingType.Property || type === e.BindingType.Class ||
type === e.BindingType.Style)) {
(type === e.BindingType.Property || type === e.BindingType.TwoWay ||
type === e.BindingType.Class || type === e.BindingType.Style)) {
// Because this binding doesn't really target the ng-template, it must be a binding on an
// inner node of a structural template. We can't skip it entirely, because we still need it on
// the ng-template's consts (e.g. for the purposes of directive matching). However, we should
Expand Down
32 changes: 19 additions & 13 deletions packages/compiler/src/template_parser/binding_parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class BindingParser {
const expression = properties[propName];
if (typeof expression === 'string') {
this.parsePropertyBinding(
propName, expression, true, sourceSpan, sourceSpan.start.offset, undefined, [],
propName, expression, true, false, sourceSpan, sourceSpan.start.offset, undefined, [],
// Use the `sourceSpan` for `keySpan`. This isn't really accurate, but neither is the
// sourceSpan, as it represents the sourceSpan of the host itself rather than the
// source of the host binding (which doesn't exist in the template). Regardless,
Expand Down Expand Up @@ -169,7 +169,8 @@ export class BindingParser {
const srcSpan = isIvyAst ? bindingSpan : sourceSpan;
const valueSpan = moveParseSourceSpan(sourceSpan, binding.value.ast.sourceSpan);
this._parsePropertyAst(
key, binding.value, srcSpan, keySpan, valueSpan, targetMatchableAttrs, targetProps);
key, binding.value, false, srcSpan, keySpan, valueSpan, targetMatchableAttrs,
targetProps);
} else {
targetMatchableAttrs.push([key, '' /* value */]);
// Since this is a literal attribute with no RHS, source span should be
Expand Down Expand Up @@ -239,8 +240,8 @@ export class BindingParser {
}

parsePropertyBinding(
name: string, expression: string, isHost: boolean, sourceSpan: ParseSourceSpan,
absoluteOffset: number, valueSpan: ParseSourceSpan|undefined,
name: string, expression: string, isHost: boolean, isPartOfAssignmentBinding: boolean,
sourceSpan: ParseSourceSpan, absoluteOffset: number, valueSpan: ParseSourceSpan|undefined,
targetMatchableAttrs: string[][], targetProps: ParsedProperty[], keySpan: ParseSourceSpan) {
if (name.length === 0) {
this._reportError(`Property name is missing in binding`, sourceSpan);
Expand Down Expand Up @@ -272,7 +273,8 @@ export class BindingParser {
} else {
this._parsePropertyAst(
name, this.parseBinding(expression, isHost, valueSpan || sourceSpan, absoluteOffset),
sourceSpan, keySpan, valueSpan, targetMatchableAttrs, targetProps);
isPartOfAssignmentBinding, sourceSpan, keySpan, valueSpan, targetMatchableAttrs,
targetProps);
}
}

Expand All @@ -284,19 +286,21 @@ export class BindingParser {
const expr = this.parseInterpolation(value, valueSpan || sourceSpan, interpolatedTokens);
if (expr) {
this._parsePropertyAst(
name, expr, sourceSpan, keySpan, valueSpan, targetMatchableAttrs, targetProps);
name, expr, false, sourceSpan, keySpan, valueSpan, targetMatchableAttrs, targetProps);
return true;
}
return false;
}

private _parsePropertyAst(
name: string, ast: ASTWithSource, sourceSpan: ParseSourceSpan, keySpan: ParseSourceSpan,
valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
targetProps: ParsedProperty[]) {
name: string, ast: ASTWithSource, isPartOfAssignmentBinding: boolean,
sourceSpan: ParseSourceSpan, keySpan: ParseSourceSpan, valueSpan: ParseSourceSpan|undefined,
targetMatchableAttrs: string[][], targetProps: ParsedProperty[]) {
targetMatchableAttrs.push([name, ast.source!]);
targetProps.push(
new ParsedProperty(name, ast, ParsedPropertyType.DEFAULT, sourceSpan, keySpan, valueSpan));
targetProps.push(new ParsedProperty(
name, ast,
isPartOfAssignmentBinding ? ParsedPropertyType.TWO_WAY : ParsedPropertyType.DEFAULT,
sourceSpan, keySpan, valueSpan));
}

private _parseAnimation(
Expand Down Expand Up @@ -387,7 +391,8 @@ export class BindingParser {
boundPropertyName = mapPropertyName ? mappedPropName : boundProp.name;
securityContexts = calcPossibleSecurityContexts(
this._schemaRegistry, elementSelector, mappedPropName, false);
bindingType = BindingType.Property;
bindingType =
boundProp.type === ParsedPropertyType.TWO_WAY ? BindingType.TwoWay : BindingType.Property;
if (!skipValidation) {
this._validatePropertyOrAttributeName(mappedPropName, boundProp.sourceSpan, false);
}
Expand Down Expand Up @@ -465,7 +470,8 @@ export class BindingParser {
const ast = this._parseAction(expression, isAssignmentEvent, handlerSpan);
targetMatchableAttrs.push([name!, ast.source!]);
targetEvents.push(new ParsedEvent(
eventName, target, ParsedEventType.Regular, ast, sourceSpan, handlerSpan, keySpan));
eventName, target, isAssignmentEvent ? ParsedEventType.TwoWay : ParsedEventType.Regular,
ast, sourceSpan, handlerSpan, keySpan));
// Don't detect directives for event names for now,
// so don't add the event name to the matchableAttrs
}
Expand Down
25 changes: 13 additions & 12 deletions packages/compiler/test/render3/r3_template_transform_spec.ts
View file
Open in desktop