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

fix(ivy): wrap functions from "providers" in parentheses in Closure mode (#33609) 

Due to the fact that Tsickle runs between analyze and transform phases in Angular, Tsickle may transform nodes (add comments with type annotations for Closure) that we captured during the analyze phase. As a result, some patterns where a function is returned from another function may trigger automatic semicolon insertion, which breaks the code (makes functions return `undefined` instead of a function). In order to avoid the problem, this commit updates the code to wrap all functions in some expression ("privders" and "viewProviders") in parentheses. More info can be found in Tsickle source code here: d797426257/src/jsdoc_transformer.ts (L1021)

PR Close #33609
This commit is contained in:
Andrew Kushnir 2019-11-05 18:00:55 -08:00 committed by Alex Rickabaugh
parent 11a35e5d14
commit fc2f6b8456
11 changed files with 251 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts
View File

@ -89,17 +89,18 @@ export class DecorationAnalyzer {
this.scopeRegistry, this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs,
/* defaultPreserveWhitespaces */ false,
/* i18nUseExternalIds */ true, /* i18nLegacyMessageIdFormat */ '', this.moduleResolver,
this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER),
this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER,
/* annotateForClosureCompiler */ false),
new DirectiveDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
this.isCore),
this.isCore, /* annotateForClosureCompiler */ false),
new InjectableDecoratorHandler(
this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore,
/* strictCtorDeps */ false),
new NgModuleDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry,
this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null,
this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER),
this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER, /* annotateForClosureCompiler */ false),
new PipeDecoratorHandler(
this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
this.isCore),

14
packages/compiler-cli/src/ngtsc/annotations/src/component.ts
View File

@ -28,7 +28,7 @@ import {ResourceLoader} from './api';
import {extractDirectiveMetadata, parseFieldArrayValue} from './directive';
import {compileNgFactoryDefField} from './factory';
import {generateSetClassMetadataCall} from './metadata';
import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, readBaseClass, unwrapExpression} from './util';
import {findAngularDecorator, isAngularCoreReference, isExpressionForwardReference, readBaseClass, unwrapExpression, wrapFunctionExpressionsInParens} from './util';
const EMPTY_MAP = new Map<string, Expression>();
const EMPTY_ARRAY: any[] = [];
@ -55,6 +55,7 @@ export class ComponentDecoratorHandler implements
private i18nLegacyMessageIdFormat: string, private moduleResolver: ModuleResolver,
private cycleAnalyzer: CycleAnalyzer, private refEmitter: ReferenceEmitter,
private defaultImportRecorder: DefaultImportRecorder,
private annotateForClosureCompiler: boolean,
private resourceDependencies:
ResourceDependencyRecorder = new NoopResourceDependencyRecorder()) {}
@ -146,7 +147,8 @@ export class ComponentDecoratorHandler implements
// on it.
const directiveResult = extractDirectiveMetadata(
node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore,
flags, this.elementSchemaRegistry.getDefaultComponentElementName());
flags, this.annotateForClosureCompiler,
this.elementSchemaRegistry.getDefaultComponentElementName());
if (directiveResult === undefined) {
// `extractDirectiveMetadata` returns undefined when the @Directive has `jit: true`. In this
// case, compilation of the decorator is skipped. Returning an empty object signifies
@ -169,7 +171,10 @@ export class ComponentDecoratorHandler implements
}, undefined) !;
const viewProviders: Expression|null = component.has('viewProviders') ?
new WrappedNodeExpr(component.get('viewProviders') !) :
new WrappedNodeExpr(
this.annotateForClosureCompiler ?
wrapFunctionExpressionsInParens(component.get('viewProviders') !) :
component.get('viewProviders') !) :
null;
// Parse the template.
@ -297,7 +302,8 @@ export class ComponentDecoratorHandler implements
i18nUseExternalIds: this.i18nUseExternalIds, relativeContextFilePath
},
metadataStmt: generateSetClassMetadataCall(
node, this.reflector, this.defaultImportRecorder, this.isCore),
node, this.reflector, this.defaultImportRecorder, this.isCore,
this.annotateForClosureCompiler),
parsedTemplate: template, parseTemplate, templateSourceMapping,
},
typeCheck: true,

20
packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
View File

@ -19,7 +19,7 @@ import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerFl
import {compileNgFactoryDefField} from './factory';
import {generateSetClassMetadataCall} from './metadata';
import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies} from './util';
import {findAngularDecorator, getConstructorDependencies, isAngularDecorator, readBaseClass, unwrapConstructorDependencies, unwrapExpression, unwrapForwardRef, validateConstructorDependencies, wrapFunctionExpressionsInParens} from './util';
const EMPTY_OBJECT: {[key: string]: string} = {};
const FIELD_DECORATORS = [
@ -40,7 +40,7 @@ export class DirectiveDecoratorHandler implements
constructor(
private reflector: ReflectionHost, private evaluator: PartialEvaluator,
private metaRegistry: MetadataRegistry, private defaultImportRecorder: DefaultImportRecorder,
private isCore: boolean) {}
private isCore: boolean, private annotateForClosureCompiler: boolean) {}
readonly precedence = HandlerPrecedence.PRIMARY;
@ -76,7 +76,7 @@ export class DirectiveDecoratorHandler implements
AnalysisOutput<DirectiveHandlerData> {
const directiveResult = extractDirectiveMetadata(
node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore,
flags);
flags, this.annotateForClosureCompiler);
const analysis = directiveResult && directiveResult.metadata;
if (analysis === undefined) {
@ -102,7 +102,8 @@ export class DirectiveDecoratorHandler implements
analysis: {
meta: analysis,
metadataStmt: generateSetClassMetadataCall(
node, this.reflector, this.defaultImportRecorder, this.isCore),
node, this.reflector, this.defaultImportRecorder, this.isCore,
this.annotateForClosureCompiler),
}
};
}
@ -136,7 +137,8 @@ export class DirectiveDecoratorHandler implements
export function extractDirectiveMetadata(
clazz: ClassDeclaration, decorator: Decorator | null, reflector: ReflectionHost,
evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean,
flags: HandlerFlags, defaultSelector: string | null = null): {
flags: HandlerFlags, annotateForClosureCompiler: boolean,
defaultSelector: string | null = null): {
decorator: Map<string, ts.Expression>,
metadata: R3DirectiveMetadata,
}|undefined {
@ -230,8 +232,12 @@ export function extractDirectiveMetadata(
const host = extractHostBindings(decoratedElements, evaluator, coreModule, directive);
const providers: Expression|null =
directive.has('providers') ? new WrappedNodeExpr(directive.get('providers') !) : null;
const providers: Expression|null = directive.has('providers') ?
new WrappedNodeExpr(
annotateForClosureCompiler ?
wrapFunctionExpressionsInParens(directive.get('providers') !) :
directive.get('providers') !) :
null;
// Determine if `ngOnChanges` is a lifecycle hook defined on the component.
const usesOnChanges = members.some(

24
packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts
View File

@ -12,7 +12,7 @@ import * as ts from 'typescript';
import {DefaultImportRecorder} from '../../imports';
import {CtorParameter, Decorator, ReflectionHost} from '../../reflection';
import {valueReferenceToExpression} from './util';
import {valueReferenceToExpression, wrapFunctionExpressionsInParens} from './util';
/**
@ -24,7 +24,7 @@ import {valueReferenceToExpression} from './util';
*/
export function generateSetClassMetadataCall(
clazz: ts.Declaration, reflection: ReflectionHost, defaultImportRecorder: DefaultImportRecorder,
isCore: boolean): Statement|null {
isCore: boolean, annotateForClosureCompiler?: boolean): Statement|null {
if (!reflection.isClass(clazz)) {
return null;
}
@ -37,7 +37,9 @@ export function generateSetClassMetadataCall(
return null;
}
const ngClassDecorators =
classDecorators.filter(dec => isAngularDecorator(dec, isCore)).map(decoratorToMetadata);
classDecorators.filter(dec => isAngularDecorator(dec, isCore))
.map(
(decorator: Decorator) => decoratorToMetadata(decorator, annotateForClosureCompiler));
if (ngClassDecorators.length === 0) {
return null;
}
@ -113,8 +115,8 @@ function ctorParameterToMetadata(
// If the parameter has decorators, include the ones from Angular.
if (param.decorators !== null) {
const ngDecorators =
param.decorators.filter(dec => isAngularDecorator(dec, isCore)).map(decoratorToMetadata);
const ngDecorators = param.decorators.filter(dec => isAngularDecorator(dec, isCore))
.map((decorator: Decorator) => decoratorToMetadata(decorator));
const value = new WrappedNodeExpr(ts.createArrayLiteral(ngDecorators));
mapEntries.push({key: 'decorators', value, quoted: false});
}
@ -126,8 +128,8 @@ function ctorParameterToMetadata(
*/
function classMemberToMetadata(
name: string, decorators: Decorator[], isCore: boolean): ts.PropertyAssignment {
const ngDecorators =
decorators.filter(dec => isAngularDecorator(dec, isCore)).map(decoratorToMetadata);
const ngDecorators = decorators.filter(dec => isAngularDecorator(dec, isCore))
.map((decorator: Decorator) => decoratorToMetadata(decorator));
const decoratorMeta = ts.createArrayLiteral(ngDecorators);
return ts.createPropertyAssignment(name, decoratorMeta);
}
@ -135,7 +137,8 @@ function classMemberToMetadata(
/**
* Convert a reflected decorator to metadata.
*/
function decoratorToMetadata(decorator: Decorator): ts.ObjectLiteralExpression {
function decoratorToMetadata(
decorator: Decorator, wrapFunctionsInParens?: boolean): ts.ObjectLiteralExpression {
if (decorator.identifier === null) {
throw new Error('Illegal state: synthesized decorator cannot be emitted in class metadata.');
}
@ -145,7 +148,10 @@ function decoratorToMetadata(decorator: Decorator): ts.ObjectLiteralExpression {
];
// Sometimes they have arguments.
if (decorator.args !== null && decorator.args.length > 0) {
const args = decorator.args.map(arg => ts.getMutableClone(arg));
const args = decorator.args.map(arg => {
const expr = ts.getMutableClone(arg);
return wrapFunctionsInParens ? wrapFunctionExpressionsInParens(expr) : expr;
});
properties.push(ts.createPropertyAssignment('args', ts.createArrayLiteral(args)));
}
return ts.createObjectLiteral(properties, true);

14
packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
View File

@ -21,7 +21,7 @@ import {getSourceFile} from '../../util/src/typescript';
import {generateSetClassMetadataCall} from './metadata';
import {ReferencesRegistry} from './references_registry';
import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression} from './util';
import {combineResolvers, findAngularDecorator, forwardRefResolver, getValidConstructorDependencies, isExpressionForwardReference, toR3Reference, unwrapExpression, wrapFunctionExpressionsInParens} from './util';
export interface NgModuleAnalysis {
mod: R3NgModuleMetadata;
@ -44,7 +44,8 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
private scopeRegistry: LocalModuleScopeRegistry,
private referencesRegistry: ReferencesRegistry, private isCore: boolean,
private routeAnalyzer: NgModuleRouteAnalyzer|null, private refEmitter: ReferenceEmitter,
private defaultImportRecorder: DefaultImportRecorder, private localeId?: string) {}
private defaultImportRecorder: DefaultImportRecorder,
private annotateForClosureCompiler: boolean, private localeId?: string) {}
readonly precedence = HandlerPrecedence.PRIMARY;
@ -212,7 +213,11 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
};
const rawProviders = ngModule.has('providers') ? ngModule.get('providers') ! : null;
const providers = rawProviders !== null ? new WrappedNodeExpr(rawProviders) : null;
const providers = rawProviders !== null ?
new WrappedNodeExpr(
this.annotateForClosureCompiler ? wrapFunctionExpressionsInParens(rawProviders) :
rawProviders) :
null;
// At this point, only add the module's imports as the injectors' imports. Any exported modules
// are added during `resolve`, as we need scope information to be able to filter out directives
@ -244,7 +249,8 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
declarations: declarationRefs,
exports: exportRefs,
metadataStmt: generateSetClassMetadataCall(
node, this.reflector, this.defaultImportRecorder, this.isCore),
node, this.reflector, this.defaultImportRecorder, this.isCore,
this.annotateForClosureCompiler),
},
factorySymbolName: node.name.text,
};

25
packages/compiler-cli/src/ngtsc/annotations/src/util.ts
View File

@ -351,3 +351,28 @@ export function readBaseClass(
return null;
}
const parensWrapperTransformerFactory: ts.TransformerFactory<ts.Expression> =
(context: ts.TransformationContext) => {
const visitor: ts.Visitor = (node: ts.Node): ts.Node => {
const visited = ts.visitEachChild(node, visitor, context);
if (ts.isArrowFunction(visited) || ts.isFunctionExpression(visited)) {
return ts.createParen(visited);
}
return visited;
};
return (node: ts.Expression) => ts.visitEachChild(node, visitor, context);
};
/**
* Wraps all functions in a given expression in parentheses. This is needed to avoid problems
* where Tsickle annotations added between analyse and transform phases in Angular may trigger
* automatic semicolon insertion, e.g. if a function is the expression in a `return` statement. More
* info can be found in Tsickle source code here:
* https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021
*
* @param expression Expression where functions should be wrapped in parentheses
*/
export function wrapFunctionExpressionsInParens(expression: ts.Expression): ts.Expression {
return ts.transform(expression, [parensWrapperTransformerFactory]).transformed[0];
}

8
packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts
View File

@ -60,9 +60,11 @@ runInEachFileSystem(() => {
const refEmitter = new ReferenceEmitter([]);
const handler = new ComponentDecoratorHandler(
reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, scopeRegistry, false,
new NoopResourceLoader(), [''], false, true, '', moduleResolver, cycleAnalyzer,
refEmitter, NOOP_DEFAULT_IMPORT_RECORDER);
reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, scopeRegistry,
/* isCore */ false, new NoopResourceLoader(), /* rootDirs */[''],
/* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true,
/* i18nLegacyMessageIdFormat */ '', moduleResolver, cycleAnalyzer, refEmitter,
NOOP_DEFAULT_IMPORT_RECORDER, /* annotateForClosureCompiler */ false);
const TestCmp = getDeclaration(program, _('/entry.ts'), 'TestCmp', isNamedClassDeclaration);
const detected = handler.detect(TestCmp, reflectionHost.getDecoratorsOfDeclaration(TestCmp));
if (detected === undefined) {

3
packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts
View File

@ -49,7 +49,8 @@ runInEachFileSystem(() => {
metaReader, new MetadataDtsModuleScopeResolver(dtsReader, null), new ReferenceEmitter([]),
null);
const handler = new DirectiveDecoratorHandler(
reflectionHost, evaluator, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER, false);
reflectionHost, evaluator, scopeRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
/* isCore */ false, /* annotateForClosureCompiler */ false);
const analyzeDirective = (dirName: string) => {
const DirNode = getDeclaration(program, _('/entry.ts'), dirName, isNamedClassDeclaration);

3
packages/compiler-cli/src/ngtsc/annotations/test/ng_module_spec.ts
View File

@ -69,7 +69,8 @@ runInEachFileSystem(() => {
const handler = new NgModuleDecoratorHandler(
reflectionHost, evaluator, metaReader, metaRegistry, scopeRegistry, referencesRegistry,
false, null, refEmitter, NOOP_DEFAULT_IMPORT_RECORDER);
false, null, refEmitter, NOOP_DEFAULT_IMPORT_RECORDER,
/* annotateForClosureCompiler */ false);
const TestModule =
getDeclaration(program, _('/entry.ts'), 'TestModule', isNamedClassDeclaration);
const detected =

8
packages/compiler-cli/src/ngtsc/program.ts
View File

@ -604,16 +604,18 @@ export class NgtscProgram implements api.Program {
this.isCore, this.resourceManager, this.rootDirs,
this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false,
this.getI18nLegacyMessageFormat(), this.moduleResolver, this.cycleAnalyzer,
this.refEmitter, this.defaultImportTracker, this.incrementalState),
this.refEmitter, this.defaultImportTracker, this.closureCompilerEnabled,
this.incrementalState),
new DirectiveDecoratorHandler(
this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore),
this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore,
this.closureCompilerEnabled),
new InjectableDecoratorHandler(
this.reflector, this.defaultImportTracker, this.isCore,
this.options.strictInjectionParameters || false),
new NgModuleDecoratorHandler(
this.reflector, evaluator, this.metaReader, metaRegistry, scopeRegistry,
referencesRegistry, this.isCore, this.routeAnalyzer, this.refEmitter,
this.defaultImportTracker, this.options.i18nInLocale),
this.defaultImportTracker, this.closureCompilerEnabled, this.options.i18nInLocale),
new PipeDecoratorHandler(
this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore),
];

147
packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
View File

@ -337,7 +337,9 @@ runInEachFileSystem(os => {
// are valid for the real OS. When on non-Windows systems it doesn't like paths
// that start with `C:`.
if (os !== 'Windows' || platform() === 'win32') {
it('should add @nocollapse to static fields when closure annotations are requested', () => {
describe('when closure annotations are requested', () => {
it('should add @nocollapse to static fields', () => {
env.tsconfig({
'annotateForClosureCompiler': true,
});
@ -357,6 +359,149 @@ runInEachFileSystem(os => {
const jsContents = env.getContents('test.js');
expect(jsContents).toContain('/** @nocollapse */ TestCmp.ɵcmp');
});
/**
* The following set of tests verify that after Tsickle run we do not have cases which
* trigger automatic semicolon insertion, which breaks the code. In order to avoid the
* problem, we wrap all function expressions in certain fields ("providers" and
* "viewProviders") in parentheses. More info on Tsickle processing related to this case can
* be found here:
* https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021
*/
describe('wrap functions in certain fields in parentheses', () => {
const providers = `
[{
provide: 'token-a',
useFactory: (service: Service) => {
return () => service.id;
}
}, {
provide: 'token-b',
useFactory: function(service: Service) {
return function() {
return service.id;
}
}
}]
`;
const service = `
export class Service {
id: string = 'service-id';
}
`;
const verifyOutput = (jsContents: string) => {
// verify that there is no pattern that triggers automatic semicolon insertion
// by checking that there are no return statements not wrapped in parentheses
expect(trim(jsContents)).not.toContain(trim(`
return /**
* @return {?}
*/
`));
expect(trim(jsContents)).toContain(trim(`
[{
provide: 'token-a',
useFactory: (function (service) {
return (/**
* @return {?}
*/
function () { return service.id; });
})
}, {
provide: 'token-b',
useFactory: (function (service) {
return (/**
* @return {?}
*/
function () {
return service.id;
});
})
}]
`));
};
it('should wrap functions in "providers" list in NgModule', () => {
env.tsconfig({
'annotateForClosureCompiler': true,
});
env.write('service.ts', service);
env.write('test.ts', `
import {NgModule} from '@angular/core';
import {Service} from './service';
@NgModule({
providers: ${providers}
})
export class SomeModule {}
`);
env.driveMain();
verifyOutput(env.getContents('test.js'));
});
it('should wrap functions in "providers" list in Component', () => {
env.tsconfig({
'annotateForClosureCompiler': true,
});
env.write('service.ts', service);
env.write('test.ts', `
import {Component} from '@angular/core';
import {Service} from './service';
@Component({
template: '...',
providers: ${providers}
})
export class SomeComponent {}
`);
env.driveMain();
verifyOutput(env.getContents('test.js'));
});
it('should wrap functions in "viewProviders" list in Component', () => {
env.tsconfig({
'annotateForClosureCompiler': true,
});
env.write('service.ts', service);
env.write('test.ts', `
import {Component} from '@angular/core';
import {Service} from './service';
@Component({
template: '...',
viewProviders: ${providers}
})
export class SomeComponent {}
`);
env.driveMain();
verifyOutput(env.getContents('test.js'));
});
it('should wrap functions in "providers" list in Directive', () => {
env.tsconfig({
'annotateForClosureCompiler': true,
});
env.write('service.ts', service);
env.write('test.ts', `
import {Directive} from '@angular/core';
import {Service} from './service';
@Directive({
providers: ${providers}
})
export class SomeDirective {}
`);
env.driveMain();
verifyOutput(env.getContents('test.js'));
});
});
});
}
it('should recognize aliased decorators', () => {