Skip to content

Commit 104302a

Browse files
author
Tim Blasi
committed
refactor(dart/transform): Remove unnecessary getter/setter codegen
Currently the transformer generates all getters and setters even when creating pre-generated change detectors, which remove the need for them. Generate getters and setters via the model provided by `ProtoViewDto`, which contains enough information to allow omitting unnecessary getters and setters from code output. Allow generating getters, setters, and method names which are Dart pseudo keywords. Closes angular#3489
1 parent ba2c077 commit 104302a

File tree

26 files changed

+791
-106
lines changed

26 files changed

+791
-106
lines changed

‎modules/angular2/src/change_detection/parser/parser.ts‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -490,14 +490,11 @@ export class _ParseAST {
490490
new MethodCall(receiver, id, fn, args);
491491

492492
} else {
493-
let getter = this.reflector.getter(id);
494-
let setter = this.reflector.setter(id);
495-
496493
if (isSafe) {
497494
if (this.optionalOperator("=")) {
498495
this.error("The '?.' operator cannot be used in the assignment");
499496
} else {
500-
return new SafePropertyRead(receiver, id, getter);
497+
return new SafePropertyRead(receiver, id, this.reflector.getter(id));
501498
}
502499
} else {
503500
if (this.optionalOperator("=")) {
@@ -506,9 +503,9 @@ export class _ParseAST {
506503
}
507504

508505
let value = this.parseConditional();
509-
return new PropertyWrite(receiver, id, setter, value);
506+
return new PropertyWrite(receiver, id, this.reflector.setter(id), value);
510507
} else {
511-
return new PropertyRead(receiver, id, getter);
508+
return new PropertyRead(receiver, id, this.reflector.getter(id));
512509
}
513510
}
514511
}

‎modules/angular2/src/core/compiler/element_injector.ts‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import {ChangeDetector, ChangeDetectorRef} from 'angular2/src/change_detection/c
4444
import {QueryList} from './query_list';
4545
import {reflector} from 'angular2/src/reflection/reflection';
4646
import {RenderDirectiveMetadata} from 'angular2/src/render/api';
47+
import {EventConfig} from 'angular2/src/render/dom/util';
4748
import {PipeBinding} from '../pipes/pipe_binding';
4849

4950
var _staticKeys;
@@ -303,18 +304,8 @@ function _createEventEmitterAccessors(bwv: BindingWithVisibility): EventEmitterA
303304
if (!(binding instanceof DirectiveBinding)) return [];
304305
var db = <DirectiveBinding>binding;
305306
return ListWrapper.map(db.eventEmitters, eventConfig => {
306-
let fieldName;
307-
let eventName;
308-
var colonIdx = eventConfig.indexOf(':');
309-
if (colonIdx > -1) {
310-
// long format: 'fieldName: eventName'
311-
fieldName = StringWrapper.substring(eventConfig, 0, colonIdx).trim();
312-
eventName = StringWrapper.substring(eventConfig, colonIdx + 1).trim();
313-
} else {
314-
// short format: 'name' when fieldName and eventName are the same
315-
fieldName = eventName = eventConfig;
316-
}
317-
return new EventEmitterAccessor(eventName, reflector.getter(fieldName));
307+
var parsedEvent = EventConfig.parse(eventConfig);
308+
return new EventEmitterAccessor(parsedEvent.eventName, reflector.getter(parsedEvent.fieldName));
318309
});
319310
}
320311

‎modules/angular2/src/render/dom/compiler/directive_parser.ts‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {CompileElement} from './compile_element';
1010
import {CompileControl} from './compile_control';
1111

1212
import {RenderDirectiveMetadata} from '../../api';
13-
import {dashCaseToCamelCase, camelCaseToDashCase, EVENT_TARGET_SEPARATOR} from '../util';
13+
import {EventConfig, dashCaseToCamelCase, camelCaseToDashCase} from '../util';
1414
import {DirectiveBuilder, ElementBinderBuilder} from '../view/proto_view_builder';
1515

1616
/**
@@ -146,12 +146,9 @@ export class DirectiveParser implements CompileStep {
146146

147147
_bindDirectiveEvent(eventName, action, compileElement, directiveBinderBuilder) {
148148
var ast = this._parser.parseAction(action, compileElement.elementDescription);
149-
if (StringWrapper.contains(eventName, EVENT_TARGET_SEPARATOR)) {
150-
var parts = eventName.split(EVENT_TARGET_SEPARATOR);
151-
directiveBinderBuilder.bindEvent(parts[1], ast, parts[0]);
152-
} else {
153-
directiveBinderBuilder.bindEvent(eventName, ast);
154-
}
149+
var parsedEvent = EventConfig.parse(eventName);
150+
var targetName = parsedEvent.isLongForm ? parsedEvent.fieldName : null;
151+
directiveBinderBuilder.bindEvent(parsedEvent.eventName, ast, targetName);
155152
}
156153

157154
_bindHostProperty(hostPropertyName, expression, compileElement, directiveBinderBuilder) {

‎modules/angular2/src/render/dom/util.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,27 @@ export function dashCaseToCamelCase(input: string): string {
2929
(m) => { return m[1].toUpperCase(); });
3030
}
3131

32+
export class EventConfig {
33+
constructor(public fieldName: string, public eventName: string, public isLongForm: boolean) {}
34+
35+
static parse(eventConfig: string): EventConfig {
36+
var fieldName = eventConfig, eventName = eventConfig, isLongForm = false;
37+
var separatorIdx = eventConfig.indexOf(EVENT_TARGET_SEPARATOR);
38+
if (separatorIdx > -1) {
39+
// long format: 'fieldName: eventName'
40+
fieldName = StringWrapper.substring(eventConfig, 0, separatorIdx).trim();
41+
eventName = StringWrapper.substring(eventConfig, separatorIdx + 1).trim();
42+
isLongForm = true;
43+
}
44+
return new EventConfig(fieldName, eventName, isLongForm);
45+
}
46+
47+
getFullName(): string {
48+
return this.isLongForm ? `${this.fieldName}${EVENT_TARGET_SEPARATOR}${this.eventName}` :
49+
this.eventName;
50+
}
51+
}
52+
3253
// Attention: This is on the hot path, so don't use closures or default values!
3354
export function queryBoundElements(templateContent: Node, isSingleElementChild: boolean):
3455
Element[] {

‎modules/angular2/src/render/dom/view/proto_view_builder.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,7 @@ export class DirectiveBuilder {
262262
}
263263
}
264264

265-
export class EventBuilder extends AstTransformer {
265+
class EventBuilder extends AstTransformer {
266266
locals: List<AST> = [];
267267
localEvents: List<Event> = [];
268268
globalEvents: List<Event> = [];

‎modules/angular2/src/transform/bind_generator/generator.dart‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ Future<String> createNgSettersAndGetters(
3131
return '$out';
3232
}
3333

34-
// TODO(kegluneq): De-dupe from template_compiler/generator.dart.
34+
// TODO(kegluneq): De-dupe from template_compiler/generator.dart, #3589.
3535

3636
/// Consumes the map generated by {@link _createPropertiesMap} to codegen
3737
/// setters.

‎modules/angular2/src/transform/common/property_utils.dart‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@ library angular2.transform.common.property_utils;
33
import 'package:analyzer/src/generated/scanner.dart' show Keyword;
44

55
/// Whether `name` is a valid property name.
6-
bool isValid(String name) => !Keyword.keywords.containsKey(name);
6+
bool isValid(String name) {
7+
var keyword = Keyword.keywords[name];
8+
return keyword == null || keyword.isPseudoKeyword;
9+
}
710

811
/// Prepares `name` to be emitted inside a string.
912
String sanitize(String name) => name.replaceAll('\$', '\\\$');

‎modules/angular2/src/transform/template_compiler/generator.dart‎

Lines changed: 18 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,9 @@ import 'package:barback/barback.dart';
2323

2424
import 'change_detector_codegen.dart' as change;
2525
import 'compile_step_factory.dart';
26-
import 'reflection_capabilities.dart';
27-
import 'reflector_register_codegen.dart' as reg;
26+
import 'reflection/codegen.dart' as reg;
27+
import 'reflection/processor.dart' as reg;
28+
import 'reflection/reflection_capabilities.dart';
2829
import 'view_definition_creator.dart';
2930

3031
/// Reads the `.ng_deps.dart` file represented by `entryPoint` and parses any
@@ -42,23 +43,22 @@ Future<String> processTemplates(AssetReader reader, AssetId entryPoint,
4243
var extractor = new _TemplateExtractor(new DomElementSchemaRegistry(),
4344
new TemplateCloner(-1), new XhrImpl(reader, entryPoint));
4445

45-
var registrations = new reg.Codegen();
46+
final processor = new reg.Processor();
47+
4648
var changeDetectorClasses = new change.Codegen();
4749
for (var rType in viewDefResults.viewDefinitions.keys) {
4850
var viewDefEntry = viewDefResults.viewDefinitions[rType];
49-
var result = await extractor.extractTemplates(viewDefEntry.viewDef);
50-
if (result == null) continue;
51+
var protoView = await extractor.extractTemplates(viewDefEntry.viewDef);
52+
if (protoView == null) continue;
5153

5254
if (generateRegistrations) {
53-
// TODO(kegluneq): Generate these getters & setters based on the
54-
// `ProtoViewDto` rather than querying the `ReflectionCapabilities`.
55-
registrations.generate(result.recording);
55+
processor.process(viewDefEntry, protoView);
5656
}
57-
if (result.protoView != null && generateChangeDetectors) {
57+
if (generateChangeDetectors) {
5858
var saved = reflector.reflectionCapabilities;
5959
reflector.reflectionCapabilities = const NullReflectionCapabilities();
6060
var defs = getChangeDetectorDefinitions(viewDefEntry.hostMetadata,
61-
result.protoView, viewDefEntry.viewDef.directives);
61+
protoView, viewDefEntry.viewDef.directives);
6262
for (var i = 0; i < defs.length; ++i) {
6363
changeDetectorClasses.generate('${rType.typeName}',
6464
'_${rType.typeName}_ChangeDetector$i', defs[i]);
@@ -67,6 +67,10 @@ Future<String> processTemplates(AssetReader reader, AssetId entryPoint,
6767
}
6868
}
6969

70+
// TODO(kegluneq): Do not hard-code `false` here once i/3436 is fixed.
71+
final registrations = new reg.Codegen(generateChangeDetectors: false);
72+
registrations.generate(processor);
73+
7074
var code = viewDefResults.ngDeps.code;
7175
if (registrations.isEmpty && changeDetectorClasses.isEmpty) return code;
7276
var importInjectIdx =
@@ -93,19 +97,16 @@ class _TemplateExtractor {
9397
ElementSchemaRegistry _schemaRegistry;
9498
TemplateCloner _templateCloner;
9599

96-
_TemplateExtractor(ElementSchemaRegistry schemaRegistry,
97-
TemplateCloner templateCloner, XHR xhr)
100+
_TemplateExtractor(this._schemaRegistry, this._templateCloner, XHR xhr)
98101
: _factory = new CompileStepFactory(new ng.Parser(new ng.Lexer())) {
99102
var urlResolver = new UrlResolver();
100103
var styleUrlResolver = new StyleUrlResolver(urlResolver);
101104
var styleInliner = new StyleInliner(xhr, styleUrlResolver, urlResolver);
102105

103106
_loader = new ViewLoader(xhr, styleInliner, styleUrlResolver);
104-
_schemaRegistry = schemaRegistry;
105-
_templateCloner = templateCloner;
106107
}
107108

108-
Future<_ExtractResult> extractTemplates(ViewDefinition viewDef) async {
109+
Future<ProtoViewDto> extractTemplates(ViewDefinition viewDef) async {
109110
// Check for "imperative views".
110111
if (viewDef.template == null && viewDef.templateAbsUrl == null) return null;
111112

@@ -115,8 +116,7 @@ class _TemplateExtractor {
115116
// operations between saving and restoring it, otherwise we can get into
116117
// a bad state. See issue #2359 for additional context.
117118
var savedReflectionCapabilities = reflector.reflectionCapabilities;
118-
var recordingCapabilities = new RecordingReflectionCapabilities();
119-
reflector.reflectionCapabilities = recordingCapabilities;
119+
reflector.reflectionCapabilities = const NullReflectionCapabilities();
120120

121121
var pipeline = new CompilePipeline(_factory.createSteps(viewDef));
122122

@@ -130,13 +130,6 @@ class _TemplateExtractor {
130130

131131
reflector.reflectionCapabilities = savedReflectionCapabilities;
132132

133-
return new _ExtractResult(recordingCapabilities, protoViewDto);
133+
return protoViewDto;
134134
}
135135
}
136-
137-
class _ExtractResult {
138-
final RecordingReflectionCapabilities recording;
139-
final ProtoViewDto protoView;
140-
141-
_ExtractResult(this.recording, this.protoView);
142-
}

modules/angular2/src/transform/template_compiler/reflector_register_codegen.dart renamed to modules/angular2/src/transform/template_compiler/reflection/codegen.dart

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,48 @@
1-
library angular2.transform.template_compiler.reflector_register_codegen;
1+
library angular2.transform.template_compiler.reflection.codegen;
22

33
import 'package:angular2/src/transform/common/names.dart';
44
import 'package:angular2/src/transform/common/property_utils.dart' as prop;
5-
import 'reflection_capabilities.dart';
5+
6+
import 'model.dart';
67

78
class Codegen {
89
final StringBuffer _buf = new StringBuffer();
910

10-
void generate(RecordingReflectionCapabilities recording) {
11-
if (recording != null) {
12-
var calls = _generateGetters(recording.getterNames);
11+
/// Whether we are pre-generating change detectors.
12+
/// If we have pre-generated change detectors, we need
13+
final bool generateChangeDetectors;
14+
15+
Codegen({this.generateChangeDetectors});
16+
17+
void generate(CodegenModel model) {
18+
if (model != null) {
19+
var calls = _generateGetters(_extractNames(model.getterNames));
1320
if (calls.isNotEmpty) {
1421
_buf.write('..${REGISTER_GETTERS_METHOD_NAME}'
1522
'({${calls.join(', ')}})');
1623
}
17-
calls = _generateSetters(recording.setterNames);
24+
calls = _generateSetters(_extractNames(model.setterNames));
1825
if (calls.isNotEmpty) {
1926
_buf.write('..${REGISTER_SETTERS_METHOD_NAME}'
2027
'({${calls.join(', ')}})');
2128
}
22-
calls = _generateMethods(recording.methodNames);
29+
calls = _generateMethods(_extractNames(model.methodNames));
2330
if (calls.isNotEmpty) {
2431
_buf.write('..${REGISTER_METHODS_METHOD_NAME}'
2532
'({${calls.join(', ')}})');
2633
}
2734
}
2835
}
2936

37+
Iterable<String> _extractNames(Iterable<ReflectiveAccessor> accessors) {
38+
var names = accessors.where((accessor) {
39+
return accessor.isStaticallyNecessary || !generateChangeDetectors;
40+
}).map((accessor) => accessor.sanitizedName);
41+
var nameList = names.toList();
42+
nameList.sort();
43+
return nameList;
44+
}
45+
3046
bool get isEmpty => _buf.isEmpty;
3147

3248
@override

‎modules/angular2/src/transform/template_compiler/reflection/model.dart‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
library angular2.transform.template_compiler.reflection.model;
2+
3+
import 'package:angular2/src/render/dom/util.dart';
4+
5+
/// Defines the names of getters, setters, and methods which need to be
6+
/// available to Angular 2 via the `reflector` at runtime.
7+
/// See [angular2.src.reflection.reflector] for details.
8+
abstract class CodegenModel {
9+
Iterable<ReflectiveAccessor> get getterNames;
10+
Iterable<ReflectiveAccessor> get methodNames;
11+
Iterable<ReflectiveAccessor> get setterNames;
12+
}
13+
14+
/// Wraps a getter, setter, or method that we may need to access reflectively in
15+
/// an Angular2 app.
16+
/// This is essentially a wrapper for `sanitizedName`, which is the name of the
17+
/// actual getter, setter, or method that will be registered. Note that
18+
/// `operator==` and `hashCode` basically forward to `sanitizedName`.
19+
class ReflectiveAccessor {
20+
/// The value in the Ast determining that we need this accessor. This is the
21+
/// value that is actually present in the template, which may not directly
22+
/// correspond to the model on the `Component`.
23+
final String astValue;
24+
25+
/// The sanitized name of this accessor. This is the name of the getter,
26+
/// setter, or method on the `Component`.
27+
final String sanitizedName;
28+
29+
/// Whether this getter, setter, or method is still necessary when we have
30+
/// pre-generated change detectors.
31+
final bool isStaticallyNecessary;
32+
33+
ReflectiveAccessor(String astValue, {this.isStaticallyNecessary})
34+
: this.astValue = astValue,
35+
this.sanitizedName = sanitizePropertyName(astValue);
36+
37+
@override
38+
bool operator ==(other) {
39+
if (other is! ReflectiveAccessor) return false;
40+
return sanitizedName == other.sanitizedName;
41+
}
42+
43+
@override
44+
int get hashCode => sanitizedName.hashCode;
45+
}
46+
47+
String sanitizePropertyName(String name) {
48+
return dashCaseToCamelCase(EventConfig.parse(name).fieldName);
49+
}

0 commit comments

Comments
 (0)