Skip to content

Commit

Permalink
refactor(compiler-cli): use ngDevMode guard for setClassMetadata
Browse files Browse the repository at this point in the history
…call (angular#39987)

Prior to this change, the `setClassMetadata` call would be invoked
inside of an IIFE that was marked as pure. This allows the call to be
tree-shaken away in production builds, as the `setClassMetadata` call
is only present to make the original class metadata available to the
testing infrastructure. The pure marker is problematic, though, as the
`setClassMetadata` call does in fact have the side-effect of assigning
the metadata into class properties. This has worked under the assumption
that only build optimization tools perform tree-shaking, however modern
bundlers are also able to elide calls that have been marked pure so this
assumption does no longer hold. Instead, an `ngDevMode` guard is used
which still allows the call to be elided but only by tooling that is
configured to consider `ngDevMode` as constant `false` value.

PR Close angular#39987
  • Loading branch information
JoostK authored and zarend committed Dec 11, 2020
1 parent f2709ff commit a272552
Show file tree
Hide file tree
Showing 55 changed files with 772 additions and 764 deletions.
3 changes: 2 additions & 1 deletion packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,8 @@ runInEachFileSystem(() => {
const jsContents = fs.readFile(_(`/node_modules/test-package/index.js`)).replace(/\s+/g, ' ');
expect(jsContents)
.toContain(
'/*@__PURE__*/ (function () { ɵngcc0.ɵsetClassMetadata(FooDirective, ' +
'(function () { (typeof ngDevMode === "undefined" || ngDevMode) && ' +
'ɵngcc0.ɵsetClassMetadata(FooDirective, ' +
'[{ type: Directive, args: [{ selector: \'[foo]\' }] }], ' +
'function () { return []; }, ' +
'{ bar: [{ type: Input }] }); })();');
Expand Down
10 changes: 6 additions & 4 deletions packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ A.ɵcmp = ɵngcc0.ɵɵdefineComponent({ type: A, selectors: [["a"]], decls: 1, v

const addAdjacentStatementsSpy = testFormatter.addAdjacentStatements as jasmine.Spy;
expect(addAdjacentStatementsSpy.calls.first().args[2]).toEqual(`// TRANSPILED
/*@__PURE__*/ (function () { ɵngcc0.ɵsetClassMetadata(A, [{
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && ɵngcc0.ɵsetClassMetadata(A, [{
type: Component,
args: [{ selector: 'a', template: '{{ person!.name }}' }]
}], null, null); })();`);
Expand Down Expand Up @@ -321,7 +321,7 @@ A.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: A, selectors: [["", "a", ""]] });`
.toEqual(jasmine.objectContaining(
{name: 'A', decorators: [jasmine.objectContaining({name: 'Directive'})]}));
expect(addAdjacentStatementsSpy.calls.first().args[2]).toEqual(`// TRANSPILED
/*@__PURE__*/ (function () { ɵngcc0.ɵsetClassMetadata(A, [{
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && ɵngcc0.ɵsetClassMetadata(A, [{
type: Directive,
args: [{ selector: '[a]' }]
}], null, null); })();`);
Expand Down Expand Up @@ -688,7 +688,8 @@ UndecoratedBase.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: UndecoratedBase, vie
decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses);
const addAdjacentStatementsSpy = testFormatter.addAdjacentStatements as jasmine.Spy;
expect(addAdjacentStatementsSpy.calls.first().args[2])
.toContain(`/*@__PURE__*/ (function () { ɵngcc0.setClassMetadata(`);
.toContain(
`function () { (typeof ngDevMode === "undefined" || ngDevMode) && ɵngcc0.setClassMetadata(`);
const addImportsSpy = testFormatter.addImports as jasmine.Spy;
expect(addImportsSpy.calls.first().args[1]).toEqual([
{specifier: './r3_symbols', qualifier: 'ɵngcc0'}
Expand All @@ -713,7 +714,8 @@ UndecoratedBase.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: UndecoratedBase, vie
decorationAnalyses, switchMarkerAnalyses, privateDeclarationsAnalyses);
const addAdjacentStatementsSpy = testFormatter.addAdjacentStatements as jasmine.Spy;
expect(addAdjacentStatementsSpy.calls.first().args[2])
.toContain(`/*@__PURE__*/ (function () { setClassMetadata(`);
.toContain(
`function () { (typeof ngDevMode === "undefined" || ngDevMode) && setClassMetadata(`);
const addImportsSpy = testFormatter.addImports as jasmine.Spy;
expect(addImportsSpy.calls.first().args[1]).toEqual([]);
});
Expand Down
15 changes: 5 additions & 10 deletions packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Expression, ExternalExpr, FunctionExpr, Identifiers, InvokeFunctionExpr, LiteralArrayExpr, LiteralExpr, literalMap, NONE_TYPE, ReturnStatement, Statement, WrappedNodeExpr} from '@angular/compiler';
import {devOnlyGuardedExpression, Expression, ExternalExpr, FunctionExpr, Identifiers, InvokeFunctionExpr, LiteralArrayExpr, LiteralExpr, literalMap, NONE_TYPE, ReturnStatement, Statement, WrappedNodeExpr} from '@angular/compiler';
import * as ts from 'typescript';

import {DefaultImportRecorder} from '../../imports';
Expand All @@ -17,7 +17,8 @@ import {valueReferenceToExpression, wrapFunctionExpressionsInParens} from './uti

/**
* Given a class declaration, generate a call to `setClassMetadata` with the Angular metadata
* present on the class or its member fields.
* present on the class or its member fields. An ngDevMode guard is used to allow the call to be
* tree-shaken away, as the `setClassMetadata` invocation is only needed for testing purposes.
*
* If no such metadata is present, this function returns `null`. Otherwise, the call is returned
* as a `Statement` for inclusion along with the class.
Expand Down Expand Up @@ -93,14 +94,8 @@ export function generateSetClassMetadataCall(
metaCtorParameters,
new WrappedNodeExpr(metaPropDecorators),
]);
const iifeFn = new FunctionExpr([], [fnCall.toStmt()], NONE_TYPE);
const iife = new InvokeFunctionExpr(
/* fn */ iifeFn,
/* args */[],
/* type */ undefined,
/* sourceSpan */ undefined,
/* pure */ true);
return iife.toStmt();
const iife = new FunctionExpr([], [devOnlyGuardedExpression(fnCall).toStmt()]);
return iife.callFn([]).toStmt();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ runInEachFileSystem(() => {
@Component('metadata') class Target {}
`);
expect(res).toEqual(
`/*@__PURE__*/ (function () { i0.ɵsetClassMetadata(Target, [{ type: Component, args: ['metadata'] }], null, null); })();`);
`(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(Target, [{ type: Component, args: ['metadata'] }], null, null); })();`);
});

it('should convert namespaced decorated class metadata', () => {
Expand All @@ -34,7 +34,7 @@ runInEachFileSystem(() => {
@core.Component('metadata') class Target {}
`);
expect(res).toEqual(
`/*@__PURE__*/ (function () { i0.ɵsetClassMetadata(Target, [{ type: core.Component, args: ['metadata'] }], null, null); })();`);
`(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(Target, [{ type: core.Component, args: ['metadata'] }], null, null); })();`);
});

it('should convert decorated class constructor parameter metadata', () => {
Expand Down

0 comments on commit a272552

Please sign in to comment.