Skip to content

Commit

Permalink
fix(compiler-cli): downlevel angular decorators to static properties
Browse files Browse the repository at this point in the history 
In v7 of Angular we removed `tsickle` from the default `ngc` pipeline.
This had the negative potential of breaking ES2015 output and SSR:

(1): TypeScript has a limitation where `forwardRef` breaks in es2015 due to
`forwardRef` calls being invoked immediately as part of decorator metadata.
See: angular/angular-cli#14424 and
angular#30106.

(2): TypeScript preserves type information for class members, and
references the type values immediately (as with `forwardRef` above).
This means that using native DOM globals as property types could
break SSR. This is because loading such JS file requires these DOM
globals to exist in NodeJS globally too (and there is no support for DI mocking).
See: angular#30586. This is especially
relevant for libraries that do not want to use tsickle but ship SSR-compatible code.

The root cause for (1) is a TypeScript limitation as mentioned. This is
the related upstream ticket: microsoft/TypeScript#27519.
Downleveling decorators to static properties fixes the issues, as
outlined in (1) and (2), because we can defer metadata evaluation
to avoid direct evaluation on file load. Additionally, we have more
control and can discard unnnecessary metadata information, like class
member types that are not needed by Angular at all see (2).

One might wonder why this hasn't been an issue in the past since we disabled
this as part of version 7. These issues didn't surface at a large scale because
we added a custom transformer to CLI projects and to `ng-packagr`. Those
transformers downleveled constructor parameters to fix (1) and (2). Also
`emitMetadataDecorator` has been disabled by default in CLI projects.
For bazel release output this didn't surface either because tsickle
still ran by default in prodmode output.

This was never an ideal solution though, and we'd also like to not
run tsickle by default in the Bazel prodmode output. It was not ideal
because we just applied workarounds at Angular compiler derivatives.
Ideally, TypeScript would just emit proper metadata that isn't evaluated
at top-level, but given they marked it as limitation and the decorator
proposal is still stage 2, this won't happen any time soon (if at all).

The ideal solution is that we downlevel decorators (as previously done
with tsickle by default) as part of the Angular compiler (a level higher;
and one below the actual TypeScript compiler limitation). This fixes
the issues with the common `forwardRef` pattern (1), and also fixes (2).
It also allows us to reduce workarounds in the compiler derivatives
(i.e. CLI and ng-packagr), and we can disable tsickle in Angular bazel
(as already done with this commit).

Fixes angular#30106. Fixes angular#30586. Fixes angular#30141.
Resolves FW-2196. Resolves FW-2199.
  • Loading branch information
@devversion
devversion committed Jun 1, 2020
1 parent 4d0e175 commit 9c6541a
Show file tree
Hide file tree
Showing 12 changed files with 1,060 additions and 342 deletions.
35 changes: 14 additions & 21 deletions packages/bazel/src/ngc-wrapped/index.ts
View file
Expand Up @@ -191,23 +191,8 @@ export function compile({
fileLoader = new UncachedFileLoader();
}

compilerOpts.annotationsAs = 'static fields';
if (!bazelOpts.es5Mode) {
if (bazelOpts.workspaceName === 'google3') {
compilerOpts.annotateForClosureCompiler = true;
} else {
compilerOpts.annotateForClosureCompiler = false;
}
}

// Detect from compilerOpts whether the entrypoint is being invoked in Ivy mode.
const isInIvyMode = !!compilerOpts.enableIvy;

// Disable downleveling and Closure annotation if in Ivy mode.
if (isInIvyMode) {
compilerOpts.annotationsAs = 'decorators';
}

if (!compilerOpts.rootDirs) {
throw new Error('rootDirs is not set!');
}
Expand Down Expand Up @@ -264,9 +249,6 @@ export function compile({
}

if (isInIvyMode) {
// Also need to disable decorator downleveling in the BazelHost in Ivy mode.
bazelHost.transformDecorators = false;

const delegate = bazelHost.shouldSkipTsickleProcessing.bind(bazelHost);
bazelHost.shouldSkipTsickleProcessing = (fileName: string) => {
// The base implementation of shouldSkipTsickleProcessing checks whether `fileName` is part of
Expand All @@ -277,12 +259,23 @@ export function compile({
};
}

// Always disable tsickle decorator transforming in the tsickle compiler host. The
// Angular compilers have their own logic for decorator processing and we wouldn't
// want tsickle to interfere with that.
bazelHost.transformDecorators = false;

// By default, annotations for closure compiler are disabled in prodmode,
// unless the workspace is known to be `google3`.
if (!bazelOpts.es5Mode && compilerOpts.annotateForClosureCompiler !== undefined) {
compilerOpts.annotateForClosureCompiler = bazelOpts.workspaceName === 'google3';
}

// The `annotateForClosureCompiler` Angular compiler option is not respected by default
// as ngc-wrapped handles tsickle emit on its own. This means that we need to update
// the tsickle compiler host based on the `annotateForClosureCompiler` flag.
if (compilerOpts.annotateForClosureCompiler) {
bazelHost.transformTypesToClosure = true;
}
if (compilerOpts.annotateForClosureCompiler || compilerOpts.annotationsAs === 'static fields') {
bazelHost.transformDecorators = true;
}

const origBazelHostFileExist = bazelHost.fileExists;
bazelHost.fileExists = (fileName: string) => {
Expand Down
5 changes: 1 addition & 4 deletions packages/bazel/test/ng_package/example_package.golden
View file
Expand Up @@ -274,7 +274,6 @@ Hello
MyService.decorators = [
{ type: i0.Injectable, args: [{ providedIn: 'root' },] }
];
/** @nocollapse */
MyService.ctorParameters = function () { return [
{ type: MySecondService }
]; };
Expand Down Expand Up @@ -605,15 +604,14 @@ let MyService = /** @class */ (() => {
MyService.decorators = [
{ type: Injectable, args: [{ providedIn: 'root' },] }
];
/** @nocollapse */
MyService.ctorParameters = () => [
{ type: MySecondService }
];
MyService.ɵprov = i0.ɵɵdefineInjectable({ factory: function MyService_Factory() { return new MyService(i0.ɵɵinject(i1.MySecondService)); }, token: MyService, providedIn: "root" });
return MyService;
})();
export { MyService };
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoicHVibGljLWFwaS5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzIjpbIi4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uL3BhY2thZ2VzL2JhemVsL3Rlc3QvbmdfcGFja2FnZS9leGFtcGxlL2ltcG9ydHMvcHVibGljLWFwaS50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQTs7Ozs7O0dBTUc7QUFFSCxPQUFPLEVBQUMsVUFBVSxFQUFDLE1BQU0sZUFBZSxDQUFDO0FBQ3pDLE9BQU8sRUFBQyxlQUFlLEVBQUMsTUFBTSxVQUFVLENBQUM7OztBQUV6QztJQUFBLE1BQ2EsU0FBUztRQUNwQixZQUFtQixhQUE4QjtZQUE5QixrQkFBYSxHQUFiLGFBQWEsQ0FBaUI7UUFBRyxDQUFDOzs7Z0JBRnRELFVBQVUsU0FBQyxFQUFDLFVBQVUsRUFBRSxNQUFNLEVBQUM7Ozs7Z0JBRnhCLGVBQWU7OztvQkFUdkI7S0FjQztTQUZZLFNBQVMiLCJzb3VyY2VzQ29udGVudCI6WyIvKipcbiAqIEBsaWNlbnNlXG4gKiBDb3B5cmlnaHQgR29vZ2xlIExMQyBBbGwgUmlnaHRzIFJlc2VydmVkLlxuICpcbiAqIFVzZSBvZiB0aGlzIHNvdXJjZSBjb2RlIGlzIGdvdmVybmVkIGJ5IGFuIE1JVC1zdHlsZSBsaWNlbnNlIHRoYXQgY2FuIGJlXG4gKiBmb3VuZCBpbiB0aGUgTElDRU5TRSBmaWxlIGF0IGh0dHBzOi8vYW5ndWxhci5pby9saWNlbnNlXG4gKi9cblxuaW1wb3J0IHtJbmplY3RhYmxlfSBmcm9tICdAYW5ndWxhci9jb3JlJztcbmltcG9ydCB7TXlTZWNvbmRTZXJ2aWNlfSBmcm9tICcuL3NlY29uZCc7XG5cbkBJbmplY3RhYmxlKHtwcm92aWRlZEluOiAncm9vdCd9KVxuZXhwb3J0IGNsYXNzIE15U2VydmljZSB7XG4gIGNvbnN0cnVjdG9yKHB1YmxpYyBzZWNvbmRTZXJ2aWNlOiBNeVNlY29uZFNlcnZpY2UpIHt9XG59XG4iXX0=
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJmaWxlIjoicHVibGljLWFwaS5qcyIsInNvdXJjZVJvb3QiOiIiLCJzb3VyY2VzIjpbIi4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uLy4uL3BhY2thZ2VzL2JhemVsL3Rlc3QvbmdfcGFja2FnZS9leGFtcGxlL2ltcG9ydHMvcHVibGljLWFwaS50cyJdLCJuYW1lcyI6W10sIm1hcHBpbmdzIjoiQUFBQTs7Ozs7O0dBTUc7QUFFSCxPQUFPLEVBQUMsVUFBVSxFQUFDLE1BQU0sZUFBZSxDQUFDO0FBQ3pDLE9BQU8sRUFBQyxlQUFlLEVBQUMsTUFBTSxVQUFVLENBQUM7OztBQUV6QztJQUFBLE1BQ2EsU0FBUztRQUNwQixZQUFtQixhQUE4QjtZQUE5QixrQkFBYSxHQUFiLGFBQWEsQ0FBaUI7UUFBRyxDQUFDOzs7Z0JBRnRELFVBQVUsU0FBQyxFQUFDLFVBQVUsRUFBRSxNQUFNLEVBQUM7OztnQkFGeEIsZUFBZTs7O29CQVR2QjtLQWNDO1NBRlksU0FBUyIsInNvdXJjZXNDb250ZW50IjpbIi8qKlxuICogQGxpY2Vuc2VcbiAqIENvcHlyaWdodCBHb29nbGUgTExDIEFsbCBSaWdodHMgUmVzZXJ2ZWQuXG4gKlxuICogVXNlIG9mIHRoaXMgc291cmNlIGNvZGUgaXMgZ292ZXJuZWQgYnkgYW4gTUlULXN0eWxlIGxpY2Vuc2UgdGhhdCBjYW4gYmVcbiAqIGZvdW5kIGluIHRoZSBMSUNFTlNFIGZpbGUgYXQgaHR0cHM6Ly9hbmd1bGFyLmlvL2xpY2Vuc2VcbiAqL1xuXG5pbXBvcnQge0luamVjdGFibGV9IGZyb20gJ0Bhbmd1bGFyL2NvcmUnO1xuaW1wb3J0IHtNeVNlY29uZFNlcnZpY2V9IGZyb20gJy4vc2Vjb25kJztcblxuQEluamVjdGFibGUoe3Byb3ZpZGVkSW46ICdyb290J30pXG5leHBvcnQgY2xhc3MgTXlTZXJ2aWNlIHtcbiAgY29uc3RydWN0b3IocHVibGljIHNlY29uZFNlcnZpY2U6IE15U2Vjb25kU2VydmljZSkge31cbn1cbiJdfQ==

--- esm2015/imports/second.js ---

Expand Down Expand Up @@ -835,7 +833,6 @@ let MyService = /** @class */ (() => {
MyService.decorators = [
{ type: Injectable, args: [{ providedIn: 'root' },] }
];
/** @nocollapse */
MyService.ctorParameters = () => [
{ type: MySecondService }
];
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/BUILD.bazel
View file
Expand Up @@ -29,6 +29,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/indexer",
"//packages/compiler-cli/src/ngtsc/perf",
"//packages/compiler-cli/src/ngtsc/reflection",
"//packages/compiler-cli/src/ngtsc/typecheck",
"@npm//@bazel/typescript",
"@npm//@types/node",
Expand Down
65 changes: 22 additions & 43 deletions packages/compiler-cli/src/main.ts
View file
Expand Up @@ -89,18 +89,9 @@ export function mainDiagnosticsForTest(
}

function createEmitCallback(options: api.CompilerOptions): api.TsEmitCallback|undefined {
const transformDecorators =
(options.enableIvy === false && options.annotationsAs !== 'decorators');
const transformTypesToClosure = options.annotateForClosureCompiler;
if (!transformDecorators && !transformTypesToClosure) {
if (!options.annotateForClosureCompiler) {
return undefined;
}
if (transformDecorators) {
// This is needed as a workaround for https://github.com/angular/tsickle/issues/635
// Otherwise tsickle might emit references to non imported values
// as TypeScript elided the import.
options.emitDecoratorMetadata = true;
}
const tsickleHost: Pick<
tsickle.TsickleHost,
'shouldSkipTsickleProcessing'|'pathToModuleName'|'shouldIgnoreWarningsForPath'|
Expand All @@ -115,41 +106,29 @@ function createEmitCallback(options: api.CompilerOptions): api.TsEmitCallback|un
googmodule: false,
untyped: true,
convertIndexImportShorthand: false,
transformDecorators,
transformTypesToClosure,
// Decorators are transformed as part of the Angular compiler programs. To avoid
// conflicts, we disable decorator transformations for tsickle.
transformDecorators: false,
transformTypesToClosure: true,
};

if (options.annotateForClosureCompiler || options.annotationsAs === 'static fields') {
return ({
program,
targetSourceFile,
writeFile,
cancellationToken,
emitOnlyDtsFiles,
customTransformers = {},
host,
options
}) =>
// tslint:disable-next-line:no-require-imports only depend on tsickle if requested
require('tsickle').emitWithTsickle(
program, {...tsickleHost, options, host, moduleResolutionHost: host}, host, options,
targetSourceFile, writeFile, cancellationToken, emitOnlyDtsFiles, {
beforeTs: customTransformers.before,
afterTs: customTransformers.after,
});
} else {
return ({
program,
targetSourceFile,
writeFile,
cancellationToken,
emitOnlyDtsFiles,
customTransformers = {},
}) =>
program.emit(
targetSourceFile, writeFile, cancellationToken, emitOnlyDtsFiles,
{after: customTransformers.after, before: customTransformers.before});
}
return ({
program,
targetSourceFile,
writeFile,
cancellationToken,
emitOnlyDtsFiles,
customTransformers = {},
host,
options
}) =>
// tslint:disable-next-line:no-require-imports only depend on tsickle if requested
require('tsickle').emitWithTsickle(
program, {...tsickleHost, options, host, moduleResolutionHost: host}, host, options,
targetSourceFile, writeFile, cancellationToken, emitOnlyDtsFiles, {
beforeTs: customTransformers.before,
afterTs: customTransformers.after,
});
}

export interface NgcParsedConfiguration extends ParsedConfiguration {
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/core/src/compiler.ts
View file
Expand Up @@ -754,7 +754,7 @@ export class NgCompiler {
/**
* Determine if the given `Program` is @angular/core.
*/
function isAngularCorePackage(program: ts.Program): boolean {
export function isAngularCorePackage(program: ts.Program): boolean {
// Look for its_just_angular.ts somewhere in the program.
const r3Symbols = getR3SymbolsFile(program);
if (r3Symbols === null) {
Expand Down

0 comments on commit 9c6541a

Please sign in to comment.