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

Decorators #7542

Closed
wants to merge 20 commits into from
Closed

Decorators #7542

wants to merge 20 commits into from

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Mar 10, 2018

EDIT by @hzoo: REPL link https://babeljs.io/repl/build/8074/ - need to turn on Stage 2

Q                       A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change? Yes
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

I'm working on #6107. Since that PR has become quite old (and there are different things that needs to be done) I'm not rebasing it, but I will just copy-paste the needed code.
Then I will set both me and @peey as the PR authors (I have no idea of how to do it, but I'm sure that it is possible 🤣).


TODO:


NOTE: The legacy-transformer.js file contains the code that was peviously in index.js, I didn't change it.


Breaking change!

I added a new option to keep both the new and legacy decorators in the same plugin. By default, the new proposal is used but it can be disabled with the legacy: true option (accepted by -syntax- and -proposal-). In the stage-0,1,2 presets this option is called decoratorsLegacy.

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories PR: WIP Spec: Decorators labels Mar 10, 2018
@babel-bot
Copy link
Collaborator

babel-bot commented Mar 10, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8074/

import syntaxDecorators from "../lib";

describe("legacy option", function() {
const oldSyntax = "@dec export class Foo {}";
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/atlassian/jest-in-case could make those assertions nicely grouped

@nicolo-ribaudo
Copy link
Member Author

The decorate helper is very big (~400 loc in helpers.js, ~6000KB in the output, ~4000KB minified). If you want to review it with proper syntax highlighting, you can check https://gist.github.com/nicolo-ribaudo/e65dc9947f8ddc5496052202b45de4b4. It's almost 1:1 with the spec.


I only implemented decorators for public methods. Since decorators transpilation is highly copuled to class fields and private properties, I think that we should have a single plugin for the three features:

{
  "plugins": [
    ["@babel/plugin-proposal-enhanced-classes", {
      "decorators": true,
      "fields": false,
      "privateMethods": true,
    }]
  ]
}

We should then deprecate @babel/plugin-proposal-decorators and leave it as-is. WDYT?

@hzoo
Copy link
Member

hzoo commented Mar 14, 2018

Yeah it might depend on https://github.com/zenparsing/js-classes-1.1 too.

I was thinking we should make things like @babel/plugin-proposal-decorators turn on the ["@babel/plugin-proposal-enhanced-classes", { "decorators": true, }] if that's possible for us implementation-wise. So on the user side it's still a plugin but internally it's in the other file.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Mar 18, 2018

After creating @babel/plugin-proposal-enhanced-classes (you can find it in this branch), I now think that it isn't a good idea because it generates too much code for classes without decorators. e.g.

`@babel/plugin-proposal-enhanced-classes` with external-helpers
const Foo = babelHelpers.enhanceClass(function(
  _defineClass,
  _initializeClass,
  _initializeInstance
) {
  _defineClass(
    class Foo {
      constructor() {
        _initializeInstance(this);
      }
    },
    [
      {
        kind: "field",
        key: "num",

        value() {
          return 2;
        },
      },
    ]
  );

  _initializeClass();
});
`@babel/plugin-proposal-enhanced-classes` without external-helpers
function _enhanceClass(factory) { var internalSlots = { F: null, elements: null, finishers: null }; factory(internalSlots, function defineClass(F, definitions) { var elements = definitions.map(_createElementDescriptor); elements = _coalesceClassElements(elements); internalSlots.F = F; internalSlots.elements = elements; }, function initializeClassElements() { _initializeClassElements(internalSlots); }, function initializeInsanceElements(O) { _initializeInstanceElements(O, internalSlots); }); return internalSlots.F; }

function _createElementDescriptor(def) { let descriptor; if (def.kind === "method") { descriptor = { value: def.value, writable: true, configurable: true }; } else if (def.kind === "get") { descriptor = { get: def.value, configurable: true }; } else if (def.kind === "set") { descriptor = { set: def.value, configurable: true }; } else if (def.kind === "field") { descriptor = { configurable: true, writable: true }; } var element = { kind: def.kind === "field" ? "field" : "method", key: def.key, placement: def.static ? "static" : def.kind === "field" ? "own" : "prototype", descriptor: descriptor }; if (def.decorators) element.decorators = def.decorators; if (def.kind === "field") element.initializer = def.value; return element; }

function _coalesceGetterSetter(element, other) { if (element.descriptor.get !== undefined) { other.descriptor.get = element.descriptor.get; } else { other.descriptor.set = element.descriptor.set; } }

function _coalesceClassElements(elements) { const newElements = []; for (var i = 0; i < elements.length; i++) { var element = elements[i]; var index = newElements.findIndex(function (other) { return other.kind === element.kind && other.key === element.key && other.placement === element.placement; }); if (element.kind === "method" && index !== -1) { var other = newElements[index]; if (element.decorators && element.decorators.length > 0) { if (other.decorators && other.decorators.length > 0) { throw new ReferenceError(); } other.decorators = element.decorators; } if (_isDataDescriptor(element.descriptor) || _isDataDescriptor(other.descriptor)) { other.descriptor = element.descriptor; } else { _coalesceGetterSetter(element, other); } } else { newElements.push(element); } } return newElements; }

function _isDataDescriptor(desc) { return desc !== undefined && !(desc.value === undefined && desc.writable === undefined); }

function _initializeClassElements(internalSlots) { var F = internalSlots.F; var proto = F.prototype; internalSlots.elements.forEach(function (element) { if (element.kind === "method") { var receiver = element.placement === "static" ? F : proto; _defineClassElement(receiver, element); } }); internalSlots.elements.forEach(function (element) { var placement = element.placement; if (element.kind === "field" && (placement === "static" || placement === "prototype")) { var receiver = placement === "static" ? F : proto; _defineClassElement(receiver, element); } }); }

function _initializeInstanceElements(O, internalSlots) { internalSlots.elements.forEach(function (element) { if (element.kind === "method" && element.placement === "own") { _defineClassElement(O, element); } }); internalSlots.elements.forEach(function (element) { if (element.kind === "field" && element.placement === "own") { _defineClassElement(O, element); } }); }

function _defineClassElement(receiver, element) { if (element.kind === "field") { var initializer = element.initializer; element.descriptor.value = initializer === void 0 ? void 0 : initializer.call(receiver); } Object.defineProperty(receiver, element.key, element.descriptor); }

const Foo = _enhanceClass(function(
  _defineClass,
  _initializeClass,
  _initializeInstance
) {
  _defineClass(
    class Foo {
      constructor() {
        _initializeInstance(this);
      }
    },
    [
      {
        kind: "field",
        key: "num",

        value() {
          return 2;
        },
      },
    ]
  );

  _initializeClass();
});
`@babel/plugin-transform-class-properties`
class Foo {
  constructor() {
    Object.defineProperty(this, "num", {
      configurable: true,
      enumerable: true,
      writable: true,
      value: 2,
    });
  }
}

I think that we should keep the two plugins separated, but let @babel/plugin-proposal-transform-decorators (which makes class fields handling a lot more complex) handle class fields in decorated classes.

I haven't tested private properties yet, but I think that they will have the same problem.

@langpavel
Copy link

Weird CircleCI error:

#!/bin/bash -eo pipefail
sudo npm i -g yarn@^1.5.1
yarn --version
/usr/local/bin/yarnpkg -> /usr/local/lib/node_modules/yarn/bin/yarn.js
/usr/local/bin/yarn -> /usr/local/lib/node_modules/yarn/bin/yarn.js
+ yarn@1.5.1
added 1 package in 0.656s
/bin/bash: line 1: /usr/local/bin/yarn: Permission denied
Exited with code 1

@nicolo-ribaudo
Copy link
Member Author

Thank you, I restarted it.

@langpavel
Copy link

Will @babel-bot update REPL?

@nicolo-ribaudo
Copy link
Member Author

Yes, the link in the first comment is always updated

@nicolo-ribaudo
Copy link
Member Author

I still need to add some tests (e.g. finishers), but this PR can be reviewed

@nicolo-ribaudo nicolo-ribaudo added the PR: Needs Review A pull request awaiting more approvals label Mar 29, 2018
@nicolo-ribaudo nicolo-ribaudo changed the title [WIP] Decorators Decorators Mar 31, 2018
@nicolo-ribaudo nicolo-ribaudo added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Mar 31, 2018
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Apr 26, 2018

Next step: #7821

After that PR, I will update Babylon again to reflect the last changes (tc39/proposal-decorators#81 (comment) and tc39/proposal-decorators#69 (comment)).

@langpavel
Copy link

langpavel commented Apr 26, 2018

Things getting complicated as I see… I was interested only in simple class or function decorators especifically only as React HoC…
Now it looks like that better patterns emerged for my use cases — render props — composition inside instead of outside…
But decorators are getting much more powerful… One big mistake from near history: Object.observe because of Angular popularity. I really hope that decorators will be much more efficient.
I believe they may be widely adopted in previous legacy version — just as really nice syntax sugar, as far as I can remember it was order of evaluation what was different against proposal…
If decorators runtime will need be large in every situation then much of people will stop using it even as old syntax sugar, because nobody wants invite tech dept to the party…

So I have questions:

  • Do you know about something that can kill performance and cannot be optimized at assembly level (V8, Chakra, …) in future?
  • Do you think that runtime can be simple again (in loose mode or after uglification for example) as it was before — with legacy decorators?

Thank you for your attention on this, I see and really appreciate — it's really hard work ♥️

@nicolo-ribaudo
Copy link
Member Author

I have merged the decorators PRs here so that they can be tested together in the repl (https://babeljs.io/repl/build/8074).

@langpavel Decorators will be hard to optimize, but I hope that js engines will figure out how to optimize them.
I will work on a loose mode in the future, but it won't be as simple as the old decorators implementation, because the new proposal is totally different and much more powerfu.

@nicolo-ribaudo
Copy link
Member Author

I'm closing this, since now it is the same as #7976

@nicolo-ribaudo nicolo-ribaudo deleted the decorators branch June 3, 2018 18:36
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release PR: Needs Review A pull request awaiting more approvals PR: New Feature 🚀 A type of pull request used for our changelog categories Priority: High Spec: Decorators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants