From bfd07b3c94ebd9f02d2561dec55d2f581c32e9cf Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Fri, 18 Oct 2019 14:45:52 +0100 Subject: [PATCH] fix(ngcc): Esm5ReflectionHost.getDeclarationOfIdentifier should handle aliased inner declarations (#33252) In ES5 modules, the class declarations consist of an IIFE with inner and outer declarations that represent the class. The `EsmReflectionHost` has logic to ensure that `getDeclarationOfIdentifier()` always returns the outer declaration. Before this commit, if an identifier referred to an alias of the inner declaration, then `getDeclarationOfIdentifier()` was failing to find the outer declaration - instead returning the inner declaration. Now the identifier is correctly resolved up to the outer declaration as expected. This should fix some of the failing 3rd party packages discussed in https://github.com/angular/ngcc-validation/issues/57. PR Close #33252 --- .../compiler-cli/ngcc/src/host/esm5_host.ts | 39 +++++++++----- .../ngcc/test/host/esm5_host_spec.ts | 52 ++++++++++++++++++- 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/esm5_host.ts b/packages/compiler-cli/ngcc/src/host/esm5_host.ts index 59a177a1a8..18570124d5 100644 --- a/packages/compiler-cli/ngcc/src/host/esm5_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm5_host.ts @@ -137,7 +137,7 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost { } const outerDeclaration = getClassDeclarationFromInnerFunctionDeclaration(declaration); - if (outerDeclaration === undefined || !hasNameIdentifier(outerDeclaration)) { + if (outerDeclaration === null || !hasNameIdentifier(outerDeclaration)) { return undefined; } @@ -162,12 +162,23 @@ export class Esm5ReflectionHost extends Esm2015ReflectionHost { * otherwise. */ getDeclarationOfIdentifier(id: ts.Identifier): Declaration|null { - // Get the identifier for the outer class node (if any). - const outerClassNode = getClassDeclarationFromInnerFunctionDeclaration(id.parent); - const declaration = super.getDeclarationOfIdentifier(outerClassNode ? outerClassNode.name : id); + const superDeclaration = super.getDeclarationOfIdentifier(id); - if (!declaration || declaration.node === null || !ts.isVariableDeclaration(declaration.node) || - declaration.node.initializer !== undefined || + if (superDeclaration === null || superDeclaration.node === null) { + return superDeclaration; + } + + // Get the identifier for the outer class node (if any). + const outerClassNode = getClassDeclarationFromInnerFunctionDeclaration(superDeclaration.node); + const declaration = outerClassNode !== null ? + super.getDeclarationOfIdentifier(outerClassNode.name) : + superDeclaration; + + if (!declaration || declaration.node === null) { + return declaration; + } + + if (!ts.isVariableDeclaration(declaration.node) || declaration.node.initializer !== undefined || // VariableDeclaration => VariableDeclarationList => VariableStatement => IIFE Block !ts.isBlock(declaration.node.parent.parent.parent)) { return declaration; @@ -528,35 +539,35 @@ function readPropertyFunctionExpression(object: ts.ObjectLiteralExpression, name * @returns the outer variable declaration or `undefined` if it is not a "class". */ function getClassDeclarationFromInnerFunctionDeclaration(node: ts.Node): - ClassDeclaration|undefined { + ClassDeclaration|null { if (ts.isFunctionDeclaration(node)) { // It might be the function expression inside the IIFE. We need to go 5 levels up... // 1. IIFE body. let outerNode = node.parent; - if (!outerNode || !ts.isBlock(outerNode)) return undefined; + if (!outerNode || !ts.isBlock(outerNode)) return null; // 2. IIFE function expression. outerNode = outerNode.parent; - if (!outerNode || !ts.isFunctionExpression(outerNode)) return undefined; + if (!outerNode || !ts.isFunctionExpression(outerNode)) return null; // 3. IIFE call expression. outerNode = outerNode.parent; - if (!outerNode || !ts.isCallExpression(outerNode)) return undefined; + if (!outerNode || !ts.isCallExpression(outerNode)) return null; // 4. Parenthesis around IIFE. outerNode = outerNode.parent; - if (!outerNode || !ts.isParenthesizedExpression(outerNode)) return undefined; + if (!outerNode || !ts.isParenthesizedExpression(outerNode)) return null; // 5. Outer variable declaration. outerNode = outerNode.parent; - if (!outerNode || !ts.isVariableDeclaration(outerNode)) return undefined; + if (!outerNode || !ts.isVariableDeclaration(outerNode)) return null; // Finally, ensure that the variable declaration has a `name` identifier. - return hasNameIdentifier(outerNode) ? outerNode : undefined; + return hasNameIdentifier(outerNode) ? outerNode : null; } - return undefined; + return null; } export function getIifeBody(declaration: ts.Declaration): ts.Block|undefined { diff --git a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts index 40abe40388..8865c5b03e 100644 --- a/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm5_host_spec.ts @@ -1817,8 +1817,58 @@ runInEachFileSystem(() => { superGetDeclarationOfIdentifierSpy.calls.reset(); expect(host.getDeclarationOfIdentifier(innerIdentifier) !.node).toBe(outerDeclaration); + expect(superGetDeclarationOfIdentifierSpy).toHaveBeenCalledWith(innerIdentifier); expect(superGetDeclarationOfIdentifierSpy).toHaveBeenCalledWith(outerIdentifier); - expect(superGetDeclarationOfIdentifierSpy).toHaveBeenCalledTimes(1); + expect(superGetDeclarationOfIdentifierSpy).toHaveBeenCalledTimes(2); + }); + + it('should return the correct outer declaration for an aliased inner class declaration inside an ES5 IIFE', + () => { + // Note that the inner class declaration `function FroalaEditorModule() {}` is aliased + // internally to `FroalaEditorModule_1`, which is used in the object returned from + // `forRoot()`. + const PROGRAM_FILE: TestFile = { + name: _('/test.js'), + contents: ` + var FroalaEditorModule = /** @class */ (function () { + function FroalaEditorModule() { + } + FroalaEditorModule_1 = FroalaEditorModule; + FroalaEditorModule.forRoot = function () { + return { ngModule: FroalaEditorModule_1, providers: [] }; + }; + var FroalaEditorModule_1; + FroalaEditorModule = FroalaEditorModule_1 = __decorate([ + NgModule({ + declarations: [FroalaEditorDirective], + exports: [FroalaEditorDirective] + }) + ], FroalaEditorModule); + return FroalaEditorModule; + }()); + export { FroalaEditorModule }; + ` + }; + + loadTestFiles([PROGRAM_FILE]); + const {program} = makeTestBundleProgram(PROGRAM_FILE.name); + const host = new Esm5ReflectionHost(new MockLogger(), false, program.getTypeChecker()); + + const expectedDeclaration = getDeclaration( + program, PROGRAM_FILE.name, 'FroalaEditorModule', isNamedVariableDeclaration); + // Grab the `FroalaEditorModule_1` identifier returned from the `forRoot()` method + const forRootMethod = ((((expectedDeclaration.initializer as ts.ParenthesizedExpression) + .expression as ts.CallExpression) + .expression as ts.FunctionExpression) + .body.statements[2] as ts.ExpressionStatement); + const identifier = + (((((forRootMethod.expression as ts.BinaryExpression).right as ts.FunctionExpression) + .body.statements[0] as ts.ReturnStatement) + .expression as ts.ObjectLiteralExpression) + .properties[0] as ts.PropertyAssignment) + .initializer as ts.Identifier; + const actualDeclaration = host.getDeclarationOfIdentifier(identifier) !; + expect(actualDeclaration.node !.getText()).toBe(expectedDeclaration.getText()); }); });