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

[Experiment] feat(7342): Decorators not allowed classes expressions #42198

Closed
wants to merge 1 commit into from

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #7342

This is an experimental PR that aims to allow using decorators in ClassExpression members. Transform idea was taken from Babel, not sure if this is an acceptable way for TypeScript.

Any feedbacks are welcome.


Babel - es5

var _class;
function decorator() { }
var foo = (_class = /*#__PURE__*/function () {
  function _class() {
    _classCallCheck(this, _class);
  }

  _createClass(_class, [{
    key: "x",
    value: function x() {}
  }]);

  return _class;
}(), (_applyDecoratedDescriptor(_class.prototype, "x", [decorator], Object.getOwnPropertyDescriptor(_class.prototype, "x"), _class.prototype)), _class);

TS - es5

var _a;
function decorator() { }
var foo = (_a = /** @class */ (function () {
    function class_1() {
    }
    class_1.prototype.x = function () { };
    return class_1;
}()), __decorate([
    decorator
], _a.prototype, "x", null), _a);

Babel - esnext

var _class;

function decorator() {}

var foo = (_class = class {
  x() {}

}, (_applyDecoratedDescriptor(_class.prototype, "x", [decorator], Object.getOwnPropertyDescriptor(_class.prototype, "x"), _class.prototype)), _class);

TS - esnext

var _a;
function decorator() { }
const foo = (_a = class {
    x() { }
}, __decorate([
    decorator
], _a.prototype, "x", null), _a);

@canonic-epicure
Copy link

Dear @a-tarasyuk, you made my day, I'm very gratitude to you. Using your branch I was able to finally generate declaration files for my project, which make heavy use of mixins + decorators (#44304)

Of course it took few hours of code tweaking, because TS is currently fragmented into 2 languages, but finally it works.

@canonic-epicure
Copy link

Btw, I noticed that I can't just use this branch in my project, directly from github, like:

  "devDependencies": {
    "typescript": "https://github.com/a-tarasyuk/TypeScript/tree/d760d2c7838404048f12433f386f99daa6dcfdac"
  },

The package is installed, and can be used, but it contains the outdated lib folder, which is updated with npx gulp LKG.

Shoudn't npx gulp LKG be part of the prepare npm script?

@a-tarasyuk
Copy link
Contributor Author

I'm not sure about the lib., @DanielRosenwasser Does the master contain the latest lib changes?
@canonic-epicure Thanks for the feedback., I would not recommend using this branch in production because these changes aren't approved or included in the next releases/milestones.

@canonic-epicure
Copy link

I have to use it, otherwise I can't generate declaration files.

Regarding npx gulp LKG in the prepare script - please nevermind, I realized, that build during installation would be taking too long.

@canonic-epicure
Copy link

But may be you can consider running npx gulp LKG and committing the updated files in lib, before pushing - that would make possible to use your branch directly. Currently I had to fork it, for that purpose.

@a-tarasyuk
Copy link
Contributor Author

@DanielRosenwasser How often do you update lib? Do you update lib before publishing to npm and don't push changes to the repo?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 3, 2021

We LKG on release-X.Y branches before a release but don't often LKG on main because we're not missing any compiler features to bootstrap ourselves. If you need an LKG, I can push one now or you can run it manually.

@a-tarasyuk
Copy link
Contributor Author

@DanielRosenwasser Is it possible to get the latest lib changes in main? Then I can merge master to this branch and @canonic-epicure can use it.

@DanielRosenwasser DanielRosenwasser mentioned this pull request Jun 3, 2021
@a-tarasyuk
Copy link
Contributor Author

@canonic-epicure I've merged main to the current branch. You can try to use it.

@canonic-epicure
Copy link

Thanks! I'll give it a try.

@a-tarasyuk
Copy link
Contributor Author

Closed in favor of waiting for the stable spec. The branch will be available, however, I think doesn't make sense to continue updating it. Summary of this feature #44555

/cc @canonic-epicure

@canonic-epicure
Copy link

@a-tarasyuk I'm very frustrated with the decision about this PR.

Just curious - what about the emerging ES decorators spec? It will still not be possible to use them in the class expressions? What do you think?

@canonic-epicure
Copy link

In any case, I'm very grateful to you for this PR, it allows me to generate the declaration files for my project. Wish there would be more contributors like you in TS world.

I'll have to stick with this branch and I guess I'll be rebasing regularly. 😅

@a-tarasyuk
Copy link
Contributor Author

The current spec allows using decorators in class expressions

Class expressions may be decorated, not just class declarations.

and Babel allows too, however, the spec still in stage 2 :(

@canonic-epicure
Copy link

Ok, so both legacy and ES decorators needs to be applicable for class expressions. This means your PR is actually future proof, as decorators emit can be abstracted, but using decorators in class expressions will remain as is.

@canonic-epicure
Copy link

@a-tarasyuk Looks like you know the TS compiler internals very well, perhaps you can check this StackOverflow question and provide some brief instructions how that can be done? Would be very much appreciated. Seems nobody knows how to do that.

@trusktr
Copy link
Contributor

trusktr commented Jul 18, 2021

I'm glad the decorators specs is finally approved to move forward. I really dislike the new @init feature though. This means end users have to worry about decorator implementation so they know whether it is meant to be called with or without @init:, which is not an ideal developer experience.

This addition would be nice, but the workaround for now is easy: create classes as statements instead. F.e.:

someAPI(class { @dec method() {} })
return class extends Foo { @dec prop = 123 }

becomes

class NewClass { @dec method() {} }
someAPI(NewClass)
class result extends Foo { @dec prop = 123 }
return result

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Decorators not allowed classes expressions
6 participants