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 7 commits into
base: master
Choose a base branch
from

Conversation

claydiffrient
Copy link
Member

@claydiffrient claydiffrient commented Oct 1, 2024

This adds a few example decorators to test if they are doing what we need them to do.

Modifies the handling of decorators to make sure they don't do anything that closure gets mad about.

The instructions here should still work for testing: #1

@claydiffrient
Copy link
Member Author

This is the error I keep getting:

❯ npx google-closure-compiler \
--js='*.js' \
--js_output_file=out.js \
--source_map_include_content \
--generate_exports \
--export_local_property_definitions \
--language_in=UNSTABLE \
--module_resolution=NODE
./using_decorators.js:1:18: WARNING - [ES6_MODULE_REFERENCES_THIS] The body of an ES6 module cannot reference this.
  1| var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
                       ^^^^

./using_decorators.js:1:26: WARNING - [ES6_MODULE_REFERENCES_THIS] The body of an ES6 module cannot reference this.
  1| var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
                               ^^^^

./using_decorators.js:21:13: ERROR - [JSC_CANNOT_CONVERT_YET] Transpilation of 'Classes with possible name shadowing' is not yet implemented.
  21| let Person = class Person {
                   ^^^^^^^^^^^^^^
  22|     name_;
      ^^^^^^^^^^
...
  43|     }
      ^^^^^
  44| };

Copy link

@ribrdb ribrdb left a comment

Choose a reason for hiding this comment

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

Do you have the ts option to import tslib? Tsickle has its own version of tslib.

@claydiffrient claydiffrient marked this pull request as ready for review October 11, 2024 16:18
@claydiffrient claydiffrient changed the title WIP: Figure out decorators Compile decorators properly when googmodule is false and transformTypesToClosure is true Oct 11, 2024
*
* This transformer fixes the problem by converting the class expression
* to a class declaration.
*/
Copy link

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

let Person = __decorate(class Person{...});

or

let Person = class Person{...}
Person = __decorate(Person)

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.

Copy link
Member Author

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:

@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}.`);
    }
  }

into:

class Person {
    name_;
    /**
     * @public
     * @param {string} name
     */
    constructor(name) {
        this.name_ = name;
    }
    /**
     * @public
     * @return {string}
     */
    get name() {
        return this.name_;
    }
    /**
     * @public
     * @return {void}
     */
    greet() {
        console.log(`Hello, my name is ${this.name}.`);
    }
};
__decorate([
    accessorDecorator(true)
], Person.prototype, JSCompiler_renameProperty("name", Person.prototype), null);
__decorate([
    methodDecorator(true)
], Person.prototype, JSCompiler_renameProperty("greet", Person.prototype), null);
/** @suppress {visibility} */
Person = __decorate([
    classDecorator
], Person);
export { Person };
/* istanbul ignore if */
if (false) {
    /**
     * @type {string}
     * @private
     */
    Person.prototype.name_;
}

Yeah I wonder if stripping the name from the class is really the right call and it'd end up just being something like

let Person = class { }

instead.

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.

Copy link

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.

class Person {...}
Person = ...

Does that work how you'd expect? Is it consistent between node, browser, closure?

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants