From 696e520842c3d2d2d9f5454875714becc72df114 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 16 Apr 2019 18:11:28 -0700 Subject: [PATCH] fix(ivy): make metadata calls side-effect free in Closure (#29947) When compiling Angular classes, the compiler may decide to append statements with specific metadata that's only required for JIT. This includes things like decorator metadata as well as NgModule scope data. When the compiler generates such calls, the call sites are marked with Uglify's PURE annotation, so the optimizer will remove them in production builds. However, Closure does not have the PURE (or similar) annotation. We have a utility function `noSideEffects` in the runtime for this purpose. This commit wraps `setClassMetadata` and `setNgModuleScope` function bodies in `noSideEffect` closures to allow Closure remove them. PR Close #29947 --- packages/core/src/render3/definition.ts | 10 ++-- packages/core/src/render3/metadata.ts | 65 +++++++++++++------------ 2 files changed, 40 insertions(+), 35 deletions(-) diff --git a/packages/core/src/render3/definition.ts b/packages/core/src/render3/definition.ts index 8bc33fd507..1293409123 100644 --- a/packages/core/src/render3/definition.ts +++ b/packages/core/src/render3/definition.ts @@ -402,10 +402,12 @@ export function ɵɵsetNgModuleScope(type: any, scope: { */ exports?: Type[] | (() => Type[]); }): void { - const ngModuleDef = getNgModuleDef(type, true); - ngModuleDef.declarations = scope.declarations || EMPTY_ARRAY; - ngModuleDef.imports = scope.imports || EMPTY_ARRAY; - ngModuleDef.exports = scope.exports || EMPTY_ARRAY; + return noSideEffects(() => { + const ngModuleDef = getNgModuleDef(type, true); + ngModuleDef.declarations = scope.declarations || EMPTY_ARRAY; + ngModuleDef.imports = scope.imports || EMPTY_ARRAY; + ngModuleDef.exports = scope.exports || EMPTY_ARRAY; + }) as never; } /** diff --git a/packages/core/src/render3/metadata.ts b/packages/core/src/render3/metadata.ts index e1e1974588..4927c671e0 100644 --- a/packages/core/src/render3/metadata.ts +++ b/packages/core/src/render3/metadata.ts @@ -7,6 +7,7 @@ */ import {Type} from '../interface/type'; +import {noSideEffects} from '../util/closure'; interface TypeWithMetadata extends Type { decorators?: any[]; @@ -26,39 +27,41 @@ interface TypeWithMetadata extends Type { export function setClassMetadata( type: Type, decorators: any[] | null, ctorParameters: (() => any[]) | null, propDecorators: {[field: string]: any} | null): void { - const clazz = type as TypeWithMetadata; + return noSideEffects(() => { + const clazz = type as TypeWithMetadata; - // We determine whether a class has its own metadata by taking the metadata from the parent - // constructor and checking whether it's the same as the subclass metadata below. We can't use - // `hasOwnProperty` here because it doesn't work correctly in IE10 for static fields that are - // defined by TS. See https://github.com/angular/angular/pull/28439#issuecomment-459349218. - const parentPrototype = clazz.prototype ? Object.getPrototypeOf(clazz.prototype) : null; - const parentConstructor: TypeWithMetadata|null = parentPrototype && parentPrototype.constructor; + // We determine whether a class has its own metadata by taking the metadata from the parent + // constructor and checking whether it's the same as the subclass metadata below. We can't use + // `hasOwnProperty` here because it doesn't work correctly in IE10 for static fields that are + // defined by TS. See https://github.com/angular/angular/pull/28439#issuecomment-459349218. + const parentPrototype = clazz.prototype ? Object.getPrototypeOf(clazz.prototype) : null; + const parentConstructor: TypeWithMetadata|null = parentPrototype && parentPrototype.constructor; - if (decorators !== null) { - if (clazz.decorators !== undefined && - (!parentConstructor || parentConstructor.decorators !== clazz.decorators)) { - clazz.decorators.push(...decorators); - } else { - clazz.decorators = decorators; + if (decorators !== null) { + if (clazz.decorators !== undefined && + (!parentConstructor || parentConstructor.decorators !== clazz.decorators)) { + clazz.decorators.push(...decorators); + } else { + clazz.decorators = decorators; + } } - } - if (ctorParameters !== null) { - // Rather than merging, clobber the existing parameters. If other projects exist which use - // tsickle-style annotations and reflect over them in the same way, this could cause issues, - // but that is vanishingly unlikely. - clazz.ctorParameters = ctorParameters; - } - if (propDecorators !== null) { - // The property decorator objects are merged as it is possible different fields have different - // decorator types. Decorators on individual fields are not merged, as it's also incredibly - // unlikely that a field will be decorated both with an Angular decorator and a non-Angular - // decorator that's also been downleveled. - if (clazz.propDecorators !== undefined && - (!parentConstructor || parentConstructor.propDecorators !== clazz.propDecorators)) { - clazz.propDecorators = {...clazz.propDecorators, ...propDecorators}; - } else { - clazz.propDecorators = propDecorators; + if (ctorParameters !== null) { + // Rather than merging, clobber the existing parameters. If other projects exist which use + // tsickle-style annotations and reflect over them in the same way, this could cause issues, + // but that is vanishingly unlikely. + clazz.ctorParameters = ctorParameters; } - } + if (propDecorators !== null) { + // The property decorator objects are merged as it is possible different fields have different + // decorator types. Decorators on individual fields are not merged, as it's also incredibly + // unlikely that a field will be decorated both with an Angular decorator and a non-Angular + // decorator that's also been downleveled. + if (clazz.propDecorators !== undefined && + (!parentConstructor || parentConstructor.propDecorators !== clazz.propDecorators)) { + clazz.propDecorators = {...clazz.propDecorators, ...propDecorators}; + } else { + clazz.propDecorators = propDecorators; + } + } + }) as never; }