forked from angular/tsickle
-
Notifications
You must be signed in to change notification settings - Fork 0
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
claydiffrient
wants to merge
7
commits into
master
Choose a base branch
from
clay/es6-decorators
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
5ebc800
WIP: Figure out decorators
claydiffrient 93b553d
WIP: handle processing decorators even with googmodules is false
claydiffrient f42c919
Use JSCompiler_rename
claydiffrient 7afa8f9
Attempt to fix downleveled decorators
claydiffrient 4034705
Make decorators work!
claydiffrient da75cc4
Update comment
claydiffrient 0213c95
Make test pass, mostly style fixes
claydiffrient File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
export function classDecorator(constructor: Function) { | ||
Object.seal(constructor); | ||
Object.seal(constructor.prototype); | ||
} | ||
|
||
export function methodDecorator(value: boolean) { | ||
return function (target: any, propertyKey: string, descriptor?: PropertyDescriptor) { | ||
if (descriptor) { | ||
descriptor.enumerable = value; | ||
} | ||
}; | ||
} | ||
|
||
export function accessorDecorator(value: boolean,) { | ||
return function (target: any, propertyKey: string, descriptor?: PropertyDescriptor) { | ||
if (descriptor) { | ||
descriptor.configurable = value; | ||
} | ||
}; | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import { accessorDecorator, classDecorator, methodDecorator } from "./decorators"; | ||
|
||
@classDecorator | ||
export class Person { | ||
private name_: string; | ||
constructor(name: string) { | ||
this.name_ = name; | ||
} | ||
|
||
@accessorDecorator(true) | ||
get name() { | ||
return this.name_; | ||
} | ||
|
||
@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_; } | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
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. | ||
*/ | ||
export function fixDownleveledDecorators() { | ||
const printer = ts.createPrinter(); | ||
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); | ||
}; | ||
}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more specific with the before/after ast?
Isn't it more like
or
How does the assignment work if you converted the class expression to a decorator? I think removing the name from the class expression is more likely to work.
Maybe we need to write a test to make sure class decorators actually work, both compiled and uncompiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm planning on writing some tests to prove out this functionality as well as the import handling.
So this ends up transforming:
into:
Yeah I wonder if stripping the name from the
class
is really the right call and it'd end up just being something likeinstead.
Closure itself doesn't have any problem compiling it as is though.... no guarantees at the moment on it actually working though post-Closure compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is I don't know what it means to re-assign name that was declared as a class.
Does that work how you'd expect? Is it consistent between node, browser, closure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how it'd work out. I'm assuming that it's okay since that's what
tsickle
was doing before when downleveling decorators. That being said, I'll see about getting some tests in place to prove it out.