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

Fixed refs to decorated class from itself methods (legacy decorators) #12007

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Amareis
Copy link
Contributor

@Amareis Amareis commented Aug 25, 2020

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

Problem described in #11131
Solved by inserting in all methods (including static, #private ones and constructor) let ClassName = _someGeneratedVariable, where _someGeneratedVariable hold "final" class, after all decorators applied.

References in class properties still isn't supported.

@babel-bot
Copy link
Collaborator

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

@Amareis Amareis changed the title Fixed refs to decorated class from itself methods Fixed refs to decorated class from itself methods (legacy decorators) Aug 25, 2020
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 77ac0ca:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration
vigilant-currying-g89qq Issue #11131

}

method() {
return Parent
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add test cases on property? i.e.

@dec
@dec2
class Parent {
  property = Parent,
}

What will p.property be? Same for private properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved by inserting in all methods (including static, #private ones and constructor) let ClassName = _someGeneratedVariable, where _someGeneratedVariable hold "final" class, after all decorators applied.
References in class properties still isn't supported.

Property isn't working currently (will hold undecorated class). For full solution we should replace all class references in read positions of class body to outer reference, but I ain't good enough at babel for such tricks.

Choose a reason for hiding this comment

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

Hi! Catch issue with property yesterday, similar to @JLHwung case, but with static property)

example source:

function dec(cls) {
  return cls
}

@dec
class Decorated {
  static property = Decorated
}

and output:

"use strict";

var _class, _class2, _temp;

function dec(cls) {
  return cls;
}

let Decorated = dec(_class = (_temp = _class2 = class Decorated {}, _class2.property = Decorated, _temp)) || _class;

This code produce runtime error ReferenceError: Cannot access 'Decorated' before initialization

Choose a reason for hiding this comment

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

@Amareis about class properties, fix references require changes in @babel/plugin-proposal-class-properties plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amareis about class properties, fix references require changes in @babel/plugin-proposal-class-properties plugin?

Don't think so. If we get full solution with replacing of original class reference in class body, this will be working without any additional efforts.

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Decorators (Legacy) labels Sep 29, 2020
@nicolo-ribaudo nicolo-ribaudo self-requested a review January 1, 2021 21:02
@xaviergonz
Copy link

xaviergonz commented Apr 15, 2021

@nicolo-ribaudo what's missing so this PR gets merged? just curious

@zachdaniel
Copy link

Would love to see this merged :)

@maccman
Copy link

maccman commented Jul 8, 2022

Let's get this merged

@nicolo-ribaudo
Copy link
Member

I was going to rebase and finish this, but I'm not sure that we should do it.

While I agree that the proposed behavior makes sense, the current behavior makes sense too:

// input.js
@dec class A { method() { A } }

// current.js
let A = dec(class A { method() { A } });

// proposed.js
let A = dec(class { method() { A } });

However, there is a big reason to keep the current behavior: changing it is not clearly a bugfix, and the code written in the past ~6 years relying on the current behavior risks to stop working.

I strongly encourage you to migrate to the current stage 3 version of the decorators proposal (https://github.com/tc39/proposal-decorators/), passing version: "2021-12" instead of version: "legacy" (or legacy: true) to the decorators plugin (https://babeljs.io/docs/en/babel-plugin-proposal-decorators#version).

@xaviergonz
Copy link

xaviergonz commented Jul 18, 2022

I'd say one good reason could be to be able to use babel to transpile typescript without a compatibility issue since this plugin is often used to do so. If the old behavior wants to be kept (understandable), then at least it could be offered as a "legacy-ts" mode or similar. Using the new stage 3 proposal would not fix the typescript compatibility issue.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Jul 18, 2022

TypeScript plans to support standard decorators in the next version: microsoft/TypeScript#49074

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Decorators (Legacy)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@babel/plugin-proposal-decorators] legacy: true, non-transformed class not picked up inside methods
8 participants