honeymoose
/
angular-cn
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor(compiler-cli): add keySpan to parsed events (#39609) 

Though we currently have the knowledge of where the `key` for an
event binding appears during parsing, we do not propagate this
information to the output AST. This means that once we produce the
template AST, we have no way of mapping a template position to the key
span alone. The best we can currently do is map back to the
`sourceSpan`. This presents problems downstream, specifically for the
language service, where we cannot provide correct information about a
position in a template because the AST is not granular enough.

This is essentially identical to the change from #38898, but for event
bindings rather than input bindings.

PR Close #39609
This commit is contained in:
Andrew Scott 2020-11-09 09:16:19 -08:00 committed by atscott
parent 21651d362d
commit c33326c538
7 changed files with 54 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
packages/compiler/src/expression_parser/ast.ts
View File

@ -869,7 +869,7 @@ export class ParsedProperty {
constructor(
public name: string, public expression: ASTWithSource, public type: ParsedPropertyType,
// TODO(atscott): `keySpan` should really be required but allows `undefined` so VE does
// TODO(FW-2095): `keySpan` should really be required but allows `undefined` so VE does
// not need to be updated. Make `keySpan` required when VE is removed.
public sourceSpan: ParseSourceSpan, readonly keySpan: ParseSourceSpan|undefined,
public valueSpan: ParseSourceSpan|undefined) {
@ -897,7 +897,8 @@ export class ParsedEvent {
constructor(
public name: string, public targetOrPhase: string, public type: ParsedEventType,
public handler: ASTWithSource, public sourceSpan: ParseSourceSpan,
public handlerSpan: ParseSourceSpan) {}
// TODO(FW-2095): keySpan should be required but was made optional to avoid changing VE
public handlerSpan: ParseSourceSpan, readonly keySpan: ParseSourceSpan|undefined) {}
}
/**

9
packages/compiler/src/render3/r3_ast.ts
View File

@ -73,14 +73,19 @@ export class BoundEvent implements Node {
constructor(
public name: string, public type: ParsedEventType, public handler: AST,
public target: string|null, public phase: string|null, public sourceSpan: ParseSourceSpan,
public handlerSpan: ParseSourceSpan) {}
public handlerSpan: ParseSourceSpan, readonly keySpan: ParseSourceSpan) {}
static fromParsedEvent(event: ParsedEvent) {
const target: string|null = event.type === ParsedEventType.Regular ? event.targetOrPhase : null;
const phase: string|null =
event.type === ParsedEventType.Animation ? event.targetOrPhase : null;
if (event.keySpan === undefined) {
throw new Error(`Unexpected state: keySpan must be defined for bound event but was not for ${
event.name}: ${event.sourceSpan}`);
}
return new BoundEvent(
event.name, event.type, event.handler, target, phase, event.sourceSpan, event.handlerSpan);
event.name, event.type, event.handler, target, phase, event.sourceSpan, event.handlerSpan,
event.keySpan);
}
visit<Result>(visitor: Visitor<Result>): Result {

22
packages/compiler/src/render3/r3_template_transform.ts
View File

@ -361,9 +361,10 @@ class HtmlAstToIvyAst implements html.Visitor {
} else if (bindParts[KW_ON_IDX]) {
const events: ParsedEvent[] = [];
const identifier = bindParts[IDENT_KW_IDX];
const keySpan = createKeySpan(srcSpan, bindParts[KW_ON_IDX], identifier);
this.bindingParser.parseEvent(
identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes,
events);
identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes, events,
keySpan);
addEvents(events, boundEvents);
} else if (bindParts[KW_BINDON_IDX]) {
const identifier = bindParts[IDENT_KW_IDX];
@ -372,7 +373,8 @@ class HtmlAstToIvyAst implements html.Visitor {
identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
matchableAttributes, parsedProperties, keySpan);
this.parseAssignmentEvent(
identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents);
identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents,
keySpan);
} else if (bindParts[KW_AT_IDX]) {
const keySpan = createKeySpan(srcSpan, '', name);
this.bindingParser.parseLiteralAttr(
@ -399,23 +401,23 @@ class HtmlAstToIvyAst implements html.Visitor {
// TODO(ayazhafiz): update this to handle malformed bindings.
name.endsWith(delims.end) && name.length > delims.start.length + delims.end.length) {
const identifier = name.substring(delims.start.length, name.length - delims.end.length);
const keySpan = createKeySpan(srcSpan, delims.start, identifier);
if (delims.start === BINDING_DELIMS.BANANA_BOX.start) {
const keySpan = createKeySpan(srcSpan, delims.start, identifier);
this.bindingParser.parsePropertyBinding(
identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
matchableAttributes, parsedProperties, keySpan);
this.parseAssignmentEvent(
identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents);
identifier, value, srcSpan, attribute.valueSpan, matchableAttributes, boundEvents,
keySpan);
} else if (delims.start === BINDING_DELIMS.PROPERTY.start) {
const keySpan = createKeySpan(srcSpan, delims.start, identifier);
this.bindingParser.parsePropertyBinding(
identifier, value, false, srcSpan, absoluteOffset, attribute.valueSpan,
matchableAttributes, parsedProperties, keySpan);
} else {
const events: ParsedEvent[] = [];
this.bindingParser.parseEvent(
identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes,
events);
identifier, value, srcSpan, attribute.valueSpan || srcSpan, matchableAttributes, events,
keySpan);
addEvents(events, boundEvents);
}
@ -463,11 +465,11 @@ class HtmlAstToIvyAst implements html.Visitor {
private parseAssignmentEvent(
name: string, expression: string, sourceSpan: ParseSourceSpan,
valueSpan: ParseSourceSpan|undefined, targetMatchableAttrs: string[][],
boundEvents: t.BoundEvent[]) {
boundEvents: t.BoundEvent[], keySpan: ParseSourceSpan) {
const events: ParsedEvent[] = [];
this.bindingParser.parseEvent(
`${name}Change`, `${expression}=$event`, sourceSpan, valueSpan || sourceSpan,
targetMatchableAttrs, events);
targetMatchableAttrs, events, keySpan);
addEvents(events, boundEvents);
}

27
packages/compiler/src/template_parser/binding_parser.ts
View File

@ -97,8 +97,14 @@ export class BindingParser {
Object.keys(dirMeta.hostListeners).forEach(propName => {
const expression = dirMeta.hostListeners[propName];
if (typeof expression === 'string') {
// TODO: pass a more accurate handlerSpan for this event.
this.parseEvent(propName, expression, sourceSpan, sourceSpan, [], targetEvents);
// Use the `sourceSpan` for `keySpan` and `handlerSpan`. 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, neither of these values are used in Ivy but are only here to satisfy the
// function signature. This should likely be refactored in the future so that `sourceSpan`
// isn't being used inaccurately.
this.parseEvent(
propName, expression, sourceSpan, sourceSpan, [], targetEvents, sourceSpan);
} else {
this._reportError(
`Value of the host listener "${
@ -408,19 +414,20 @@ export class BindingParser {
boundProp.sourceSpan, boundProp.keySpan, boundProp.valueSpan);
}
// TODO: keySpan should be required but was made optional to avoid changing VE parser.
parseEvent(
name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
targetMatchableAttrs: string[][], targetEvents: ParsedEvent[]) {
targetMatchableAttrs: string[][], targetEvents: ParsedEvent[], keySpan?: ParseSourceSpan) {
if (name.length === 0) {
this._reportError(`Event name is missing in binding`, sourceSpan);
}
if (isAnimationLabel(name)) {
name = name.substr(1);
this._parseAnimationEvent(name, expression, sourceSpan, handlerSpan, targetEvents);
this._parseAnimationEvent(name, expression, sourceSpan, handlerSpan, targetEvents, keySpan);
} else {
this._parseRegularEvent(
name, expression, sourceSpan, handlerSpan, targetMatchableAttrs, targetEvents);
name, expression, sourceSpan, handlerSpan, targetMatchableAttrs, targetEvents, keySpan);
}
}
@ -432,7 +439,7 @@ export class BindingParser {
private _parseAnimationEvent(
name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
targetEvents: ParsedEvent[]) {
targetEvents: ParsedEvent[], keySpan?: ParseSourceSpan) {
const matches = splitAtPeriod(name, [name, '']);
const eventName = matches[0];
const phase = matches[1].toLowerCase();
@ -442,7 +449,7 @@ export class BindingParser {
case 'done':
const ast = this._parseAction(expression, handlerSpan);
targetEvents.push(new ParsedEvent(
eventName, phase, ParsedEventType.Animation, ast, sourceSpan, handlerSpan));
eventName, phase, ParsedEventType.Animation, ast, sourceSpan, handlerSpan, keySpan));
break;
default:
@ -462,13 +469,13 @@ export class BindingParser {
private _parseRegularEvent(
name: string, expression: string, sourceSpan: ParseSourceSpan, handlerSpan: ParseSourceSpan,
targetMatchableAttrs: string[][], targetEvents: ParsedEvent[]) {
targetMatchableAttrs: string[][], targetEvents: ParsedEvent[], keySpan?: ParseSourceSpan) {
// long format: 'target: eventName'
const [target, eventName] = splitAtColon(name, [null!, name]);
const ast = this._parseAction(expression, handlerSpan);
targetMatchableAttrs.push([name!, ast.source!]);
targetEvents.push(
new ParsedEvent(eventName, target, ParsedEventType.Regular, ast, sourceSpan, handlerSpan));
targetEvents.push(new ParsedEvent(
eventName, target, 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
}

18
packages/compiler/test/render3/r3_ast_spans_spec.ts
View File

@ -78,8 +78,10 @@ class R3AstSourceSpans implements t.Visitor<void> {
}
visitBoundEvent(event: t.BoundEvent) {
this.result.push(
['BoundEvent', humanizeSpan(event.sourceSpan), humanizeSpan(event.handlerSpan)]);
this.result.push([
'BoundEvent', humanizeSpan(event.sourceSpan), humanizeSpan(event.keySpan),
humanizeSpan(event.handlerSpan)
]);
}
visitText(text: t.Text) {
@ -356,21 +358,21 @@ describe('R3 AST source spans', () => {
it('is correct for event names case sensitive', () => {
expectFromHtml('<div (someEvent)="v"></div>').toEqual([
['Element', '<div (someEvent)="v"></div>', '<div (someEvent)="v">', '</div>'],
['BoundEvent', '(someEvent)="v"', 'v'],
['BoundEvent', '(someEvent)="v"', 'someEvent', 'v'],
]);
});
it('is correct for bound events via on-', () => {
expectFromHtml('<div on-event="v"></div>').toEqual([
['Element', '<div on-event="v"></div>', '<div on-event="v">', '</div>'],
['BoundEvent', 'on-event="v"', 'v'],
['BoundEvent', 'on-event="v"', 'event', 'v'],
]);
});
it('is correct for bound events via data-on-', () => {
expectFromHtml('<div data-on-event="v"></div>').toEqual([
['Element', '<div data-on-event="v"></div>', '<div data-on-event="v">', '</div>'],
['BoundEvent', 'data-on-event="v"', 'v'],
['BoundEvent', 'data-on-event="v"', 'event', 'v'],
]);
});
@ -378,7 +380,7 @@ describe('R3 AST source spans', () => {
expectFromHtml('<div [(prop)]="v"></div>').toEqual([
['Element', '<div [(prop)]="v"></div>', '<div [(prop)]="v">', '</div>'],
['BoundAttribute', '[(prop)]="v"', 'prop', 'v'],
['BoundEvent', '[(prop)]="v"', 'v'],
['BoundEvent', '[(prop)]="v"', 'prop', 'v'],
]);
});
@ -386,7 +388,7 @@ describe('R3 AST source spans', () => {
expectFromHtml('<div bindon-prop="v"></div>').toEqual([
['Element', '<div bindon-prop="v"></div>', '<div bindon-prop="v">', '</div>'],
['BoundAttribute', 'bindon-prop="v"', 'prop', 'v'],
['BoundEvent', 'bindon-prop="v"', 'v'],
['BoundEvent', 'bindon-prop="v"', 'prop', 'v'],
]);
});
@ -394,7 +396,7 @@ describe('R3 AST source spans', () => {
expectFromHtml('<div data-bindon-prop="v"></div>').toEqual([
['Element', '<div data-bindon-prop="v"></div>', '<div data-bindon-prop="v">', '</div>'],
['BoundAttribute', 'data-bindon-prop="v"', 'prop', 'v'],
['BoundEvent', 'data-bindon-prop="v"', 'v'],
['BoundEvent', 'data-bindon-prop="v"', 'prop', 'v'],
]);
});
});

4
packages/language-service/ivy/test/definitions_spec.ts
View File

@ -199,7 +199,7 @@ describe('definitions', () => {
it('should work for event providers', () => {
const definitions = getDefinitionsAndAssertBoundSpan({
templateOverride: `<test-comp (te¦st)="myClick($event)"></test-comp>`,
expectedSpanText: '(test)="myClick($event)"',
expectedSpanText: 'test',
});
expect(definitions!.length).toEqual(1);
@ -218,7 +218,7 @@ describe('definitions', () => {
it('should return the directive when the event is part of the selector', () => {
const definitions = getDefinitionsAndAssertBoundSpan({
templateOverride: `<div (eventSelect¦or)="title = ''"></div>`,
expectedSpanText: `(eventSelector)="title = ''"`,
expectedSpanText: `eventSelector`,
});
expect(definitions!.length).toEqual(2);

6
packages/language-service/ivy/test/quick_info_spec.ts
View File

@ -156,7 +156,7 @@ describe('quick info', () => {
it('should work for event providers', () => {
expectQuickInfo({
templateOverride: `<test-comp (te¦st)="myClick($event)"></test-comp>`,
expectedSpanText: '(test)="myClick($event)"',
expectedSpanText: 'test',
expectedDisplayString: '(event) TestComponent.testEvent: EventEmitter<any>'
});
});
@ -164,12 +164,12 @@ describe('quick info', () => {
it('should work for on- syntax binding', () => {
expectQuickInfo({
templateOverride: `<test-comp on-te¦st="myClick($event)"></test-comp>`,
expectedSpanText: 'on-test="myClick($event)"',
expectedSpanText: 'test',
expectedDisplayString: '(event) TestComponent.testEvent: EventEmitter<any>'
});
expectQuickInfo({
templateOverride: `<test-comp data-on-te¦st="myClick($event)"></test-comp>`,
expectedSpanText: 'data-on-test="myClick($event)"',
expectedSpanText: 'test',
expectedDisplayString: '(event) TestComponent.testEvent: EventEmitter<any>'
});
});