Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compile decorators properly when googmodule is false and transformTypesToClosure is true #2

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
33 changes: 33 additions & 0 deletions demo/decorators.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
export const classDecorator = (constructor: Function) => {
Object.seal(constructor);
Object.seal(constructor.prototype);
};

export const methodDecorator = (value: boolean) =>{
return (target: unknown, propertyKey: string, descriptor?: PropertyDescriptor) => {
if (descriptor) {
descriptor.enumerable = value;
}
};
};

export const accessorDecorator = (value: boolean) => {
return (target: unknown, propertyKey: string, descriptor?: PropertyDescriptor) => {
if (descriptor) {
descriptor.configurable = value;
}
claydiffrient marked this conversation as resolved.
Show resolved Hide resolved
};
};

/**
* @Annotation
* @param {boolean} value
* @return {function(?, string, (PropertyDescriptor|undefined)): void}
*/
export const annotatedAccessorDecorator = (value: boolean) => {
return (target: unknown, propertyKey: string, descriptor?: PropertyDescriptor) => {
if (descriptor) {
descriptor.configurable = value;
}
};
};
2 changes: 1 addition & 1 deletion demo/demo_exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function baz() : SpecialType {
export class MyClass {
constructor(private objName: string) {}

public getName() {
getName() {
return this.objName;
}
}
4 changes: 3 additions & 1 deletion demo/demo_star_exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import type { SpecialType } from "./demo_exports";

export * from "./demo_exports";

export * as decorators from "./decorators";

const myObj = new MyClass("myObj");

const bazValue: SpecialType = baz()
const bazValue: SpecialType = baz();
1 change: 1 addition & 0 deletions demo/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"strict": true,
"moduleResolution": "Bundler",
"esModuleInterop": true,
"experimentalDecorators": true,
},
"exclude": [
"test.ts"
Expand Down
33 changes: 33 additions & 0 deletions demo/using_decorators.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { accessorDecorator, annotatedAccessorDecorator, classDecorator, methodDecorator } from "./decorators";

@classDecorator
export class Person {
private name_: string;
constructor(name: string) {
this.name_ = name;
}

@accessorDecorator(true)
get name() {
return this.name_;
}

@annotatedAccessorDecorator(true)
get age() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the demo this is getting emitted as a normal decorator, not converted to an annotation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In demo/tsickle-out/using_decorators.js I'm seeing it get output as:

/** @type {!Object<string, !Array<{type: !Function, args: (undefined|!Array<?>)}>>} */
static propDecorators = {
    age: [{ type: annotatedAccessorDecorator, args: [true,] }]
};

whereas the other decorators get output as:

__decorate([
    accessorDecorator(true)
], Person.prototype, JSCompiler_renameProperty("name", Person.prototype), null);

is that not the correct behavior?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, when I followed the testing directions from the other pr I get this output:

__decorate([
    annotatedAccessorDecorator(true)
], Person.prototype, "age", null);

I also just noticed __decorate is defined inline instead of being imported too, which doesn't seem right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What??? That doesn't make any sense :(

return 42;
}

@methodDecorator(true)
greet() {
console.log(`Hello, my name is ${this.name}.`);
}
}

const p = new Person("Ron");
p.greet();


export class Employee {
constructor(private age_: number) {}
get age() { return this.age_; }
}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"@types/source-map-support": "^0.5.3",
"diff-match-patch": "^1.0.5",
"glob": "8.0.1",
"google-closure-compiler": "^20230411.0.0",
"google-closure-compiler": "^20230502.0.0",
"jasmine": "^4.1.0",
"jasmine-node": "^3.0.0",
"source-map": "^0.7.3",
Expand Down
71 changes: 43 additions & 28 deletions src/decorators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,12 @@ function isExportingDecorator(decorator: ts.Decorator, typeChecker: ts.TypeCheck
* ], Foo.prototype,
* __googReflect.objectProperty("prop", Foo.prototype), void 0);
*/
export function transformDecoratorsOutputForClosurePropertyRenaming(diagnostics: ts.Diagnostic[]) {
export function transformDecoratorsOutputForClosurePropertyRenaming(diagnostics: ts.Diagnostic[], useGoogModule = true) {
return (context: ts.TransformationContext) => {
const result: ts.Transformer<ts.SourceFile> = (sourceFile: ts.SourceFile) => {
let nodeNeedingGoogReflect: undefined|ts.Node = undefined;
const visitor: ts.Visitor = (node) => {
const replacementNode = rewriteDecorator(node);
const replacementNode = rewriteDecorator(node, useGoogModule);
if (replacementNode) {
nodeNeedingGoogReflect = node;
return replacementNode;
Expand All @@ -113,30 +113,34 @@ export function transformDecoratorsOutputForClosurePropertyRenaming(diagnostics:
if (nodeNeedingGoogReflect !== undefined) {
const statements = [...updatedSourceFile.statements];
const googModuleIndex = statements.findIndex(isGoogModuleStatement);
if (googModuleIndex === -1) {
if (useGoogModule && googModuleIndex === -1) {
reportDiagnostic(
diagnostics, nodeNeedingGoogReflect,
'Internal tsickle error: could not find goog.module statement to import __tsickle_googReflect for decorator compilation.');
return sourceFile;
}
const googRequireReflectObjectProperty =
ts.factory.createVariableStatement(
undefined,
ts.factory.createVariableDeclarationList(
[ts.factory.createVariableDeclaration(
'__tsickle_googReflect',
/* exclamationToken */ undefined, /* type */ undefined,
ts.factory.createCallExpression(
ts.factory.createPropertyAccessExpression(
ts.factory.createIdentifier('goog'), 'require'),
undefined,
[ts.factory.createStringLiteral('goog.reflect')]))],
ts.NodeFlags.Const));
// The boilerplate we produce has a goog.module line, then two related
// lines dealing with the `module` variable. Insert our goog.require
// after that to avoid visually breaking up the module info, and to be
// with the rest of the goog.require statements.
statements.splice(googModuleIndex + 3, 0, googRequireReflectObjectProperty);
if (useGoogModule) {
const googRequireReflectObjectProperty =
ts.factory.createVariableStatement(
undefined,
ts.factory.createVariableDeclarationList(
[ts.factory.createVariableDeclaration(
'__tsickle_googReflect',
/* exclamationToken */ undefined, /* type */ undefined,
ts.factory.createCallExpression(
ts.factory.createPropertyAccessExpression(
ts.factory.createIdentifier('goog'), 'require'),
undefined,
[ts.factory.createStringLiteral('goog.reflect')]))],
ts.NodeFlags.Const));


// The boilerplate we produce has a goog.module line, then two related
// lines dealing with the `module` variable. Insert our goog.require
// after that to avoid visually breaking up the module info, and to be
// with the rest of the goog.require statements.
statements.splice(googModuleIndex + 3, 0, googRequireReflectObjectProperty);
}
updatedSourceFile = ts.factory.updateSourceFile(
updatedSourceFile,
ts.setTextRange(
Expand All @@ -161,7 +165,7 @@ export function transformDecoratorsOutputForClosurePropertyRenaming(diagnostics:
*
* Returns undefined if no modification is necessary.
*/
function rewriteDecorator(node: ts.Node): ts.Node|undefined {
function rewriteDecorator(node: ts.Node, useGoogModule = true): ts.Node|undefined {
if (!ts.isCallExpression(node)) {
return;
}
Expand All @@ -188,12 +192,23 @@ function rewriteDecorator(node: ts.Node): ts.Node|undefined {
return;
}
const fieldNameLiteral = untypedFieldNameLiteral;
args[2] = ts.factory.createCallExpression(
ts.factory.createPropertyAccessExpression(
ts.factory.createIdentifier('__tsickle_googReflect'),
'objectProperty'),
undefined,
[ts.factory.createStringLiteral(fieldNameLiteral.text), args[1]]);
if (useGoogModule) {
args[2] = ts.factory.createCallExpression(
ts.factory.createPropertyAccessExpression(
ts.factory.createIdentifier('__tsickle_googReflect'),
'objectProperty'),
undefined,
[ts.factory.createStringLiteral(fieldNameLiteral.text), args[1]]);
} else {
args[2] = ts.factory.createCallExpression(
ts.factory.createIdentifier('JSCompiler_renameProperty'),
undefined,
[
ts.factory.createStringLiteral(fieldNameLiteral.text),
args[1],
],
);
}
return ts.factory.updateCallExpression(
node, node.expression, node.typeArguments, args);
}
Expand Down
59 changes: 59 additions & 0 deletions src/export_star_transformer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import * as ts from "typescript";

/**
* Closure compiler cannot handle:
*
* ```
* export * as namespace from "./module";
* ```
*
* This transformer changes star namespace exports to the following:
*
* ```
* import * as namespace from "./module";
* export {namespace};
claydiffrient marked this conversation as resolved.
Show resolved Hide resolved
* ```
*/
export function exportStarTransformer() {
return (context: ts.TransformationContext): ts.Transformer<ts.SourceFile> => {
return (sourceFile: ts.SourceFile) => {
function visit(node: ts.Node): ts.Node | ts.Node[] {
// Check if the node is a NamespaceExport (export * as NS from "module")
if (ts.isExportDeclaration(node) && node.exportClause && ts.isNamespaceExport(node.exportClause)) {
const namespaceName = node.exportClause.name;
const moduleSpecifier = node.moduleSpecifier;

if (!moduleSpecifier) return node;

// Create import * as namespace from "module"
const importDecl = ts.factory.createImportDeclaration(
undefined,
ts.factory.createImportClause(
false,
undefined,
ts.factory.createNamespaceImport(namespaceName)
),
moduleSpecifier
);

// Create export {namespace}
const exportDecl = ts.factory.createExportDeclaration(
undefined,
false,
ts.factory.createNamedExports([
ts.factory.createExportSpecifier(
false,
undefined,
namespaceName
)
])
);

return [importDecl, exportDecl];
}
return ts.visitEachChild(node, visit, context);
}
return ts.visitEachChild(sourceFile, visit, context);
};
};
}
54 changes: 54 additions & 0 deletions src/fix_downleveled_decorators.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import * as ts from "typescript";

/**
* This fixes the downleveled decorators so Closure doesn't have
* trouble with them. The problem is that when `experimentalDecorators` is
* enabled, we TSC ends up converting a class decorator like this:
*
* @classDecorator
* export class Person { ... }
*
* to this:
*
* let Person = class Person { ... };
*
* as well as some calls to __decorate(Person, ...)
*
* The problem is that this causes Closure Compiler to fail with this error:
* ERROR - [JSC_CANNOT_CONVERT_YET] Transpilation of 'Classes with possible name shadowing' is not yet implemented.
* 21| let Person = class Person {
* ^^^^^^^^^^^^^^
*
* This transformer fixes the problem by converting the class expression
* to a class declaration.
*/
claydiffrient marked this conversation as resolved.
Show resolved Hide resolved
export function fixDownleveledDecorators() {
return (context: ts.TransformationContext): ts.Transformer<ts.SourceFile> => {
return (sourceFile: ts.SourceFile) => {
function visit(node: ts.Node): ts.Node {
// Check if the node is a VariableDeclarationList
if (ts.isVariableDeclarationList(node)) {
for (const declaration of node.declarations) {
if (
declaration.initializer &&
ts.isClassExpression(declaration.initializer)
) {
const className = declaration.name;
// convert the class expression to a class declaration
const classDeclaration = ts.factory.createClassDeclaration(
undefined,
className.getText(),
[],
[],
declaration.initializer.members
);
return classDeclaration;
}
}
}
return ts.visitEachChild(node, visit, context);
}
return ts.visitEachChild(sourceFile, visit, context);
};
};
}
2 changes: 1 addition & 1 deletion src/jsdoc_transformer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1429,7 +1429,7 @@ export function jsdocTransformer(
}

function visitor(node: ts.Node): ts.Node|ts.Node[] {
const leaveModulesAlone = !host.googmodule && host.transformTypesToClosure
const leaveModulesAlone = !host.googmodule && host.transformTypesToClosure;
if (transformerUtil.isAmbient(node)) {
if (!transformerUtil.hasModifierFlag(node as ts.Declaration, ts.ModifierFlags.Export)) {
return node;
Expand Down
9 changes: 9 additions & 0 deletions src/tsickle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import {FileSummary, SummaryGenerationProcessorHost} from './summary';
import {isDtsFileName} from './transformer_util';
import * as tsmes from './ts_migration_exports_shim';
import {makeTsickleDeclarationMarkerTransformerFactory} from './tsickle_declaration_marker';
import {fixDownleveledDecorators} from './fix_downleveled_decorators';
import { exportStarTransformer } from './export_star_transformer';

// Exported for users as a default impl of pathToModuleName.
export {pathToModuleName} from './cli_support';
Expand Down Expand Up @@ -248,6 +250,13 @@ export function emit(
transformDecoratorsOutputForClosurePropertyRenaming(
tsickleDiagnostics));
tsTransformers.after!.push(transformDecoratorJsdoc());
} else if (host.transformTypesToClosure) {
tsTransformers.after!.push(exportStarTransformer());
tsTransformers.after!.push(
transformDecoratorsOutputForClosurePropertyRenaming(
tsickleDiagnostics, false));
tsTransformers.after!.push(transformDecoratorJsdoc());
tsTransformers.after!.push(fixDownleveledDecorators());
}
if (host.addDtsClutzAliases) {
tsTransformers.afterDeclarations!.push(
Expand Down
Loading