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

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

Open
xaviergonz opened this issue Feb 12, 2020 · 4 comments

Comments

@xaviergonz
Copy link

@xaviergonz xaviergonz commented Feb 12, 2020

Bug Report

  • I would like to work on a fix!

Current Behavior

When using legacy: true (didn't check without it) and using a class decorator that transforms the class into another one ("wraps it"), the original class will be used instead of the original one.

Input Code

https://codesandbox.io/s/boring-morning-yeel2

function myClassDecorator(clazz) {
  return class extends clazz {};
}

function getA() {
  // here it returns the wrapped class correctly
  return A;
}

@myClassDecorator
class A {
  getA1() {
    // apparently in this case it will pick up the original class rather than the transformed one
    return A;
  }

  getA2() {
    return getA(); // should be no different than `getA1`, but it is :-/
    // in this case it correctly returns the wrapped class
  }

  getA3 = () => {
    // apparently in this case it will pick up the original class rather than the transformed one
    return A;
  };
}

const a1 = new A().getA1();
if (a1 !== A) {
  console.error("a1: different class"); // triggers
}

const a2 = new A().getA2();
if (a2 !== A) {
  console.error("a2: different class"); // does not trigger
}

const a3 = new A().getA3();
if (a3 !== A) {
  console.error("a3: different class"); // triggers
}

Expected behavior/code
I expect it to return / use the wrapped class rather than the original one in all instances (like TS does)

Babel Configuration (babel.config.js, .babelrc, package.json#babel, cli command, .eslintrc)

babel-preset-react-app package when running in TS mode
(basically @babel/plugin-proposal-decorators with legacy set to true)

Environment

Babel version(s): core 7.8.4, babel/plugin-proposal-decorators 7.8.3

System:
    OS: Windows 10 10.0.18363
  Binaries:
    Node: 13.6.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.0 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
    npm: 6.13.4 - C:\Program Files\nodejs\npm.CMD

Possible Solution

The generated code:

var _class, _temp;

function myClassDecorator(clazz) {
  return class extends clazz {};
}

function getA() {
  return A;
}

let A = myClassDecorator(_class = (_temp = class A {
  constructor() {
    this.getA3 = () => {
      // apparently in this case it will pick up the original class rather than the transformed one
      return A;
    };
  }

  getA1() {
    // apparently in this case it will pick up the original class rather than the transformed one
    return A;
  }

  getA2() {
    return getA(); // should be no different than `getA1`, but it is :-/
  }

}, _temp)) || _class;

const a1 = new A().getA1();

if (a1 !== A) {
  console.error("a1: different class"); // triggers
}

const a2 = new A().getA2();

if (a2 !== A) {
  console.error("a2: different class"); // does not trigger
}

const a3 = new A().getA3();

if (a3 !== A) {
  console.error("a3: different class"); // triggers
}

this line:
let A = myClassDecorator(_class = (_temp = class A {

makes me think the method is picking up the class A rather than the let A, so maybe the solution could be to do what TS does:

Playground Link

...
var A_1;
function myClassDecorator(clazz) {
    return class extends clazz {
    };
}
function getA() {
    return A;
}
let A = A_1 = class A {
    constructor() {
        this.getA3 = () => {
            // apparently in this case it will pick up the original class rather than the transformed one
            return A_1;
        };
    }
    getA1() {
        // apparently in this case it will pick up the original class rather than the transformed one
        return A_1;
    }
    getA2() {
        return getA(); // should be no different than `getA1`, but it is :-/
    }
};
A = A_1 = __decorate([
    myClassDecorator
], A);

This is, to replace

let A = myClassDecorator(_class = (_temp = class A {

with

var A_1;
let A = A_1 = myClassDecorator(_class = (_temp = class A {

and any usages of A inside class A to be A_1 instead

@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Feb 12, 2020

Hey @xaviergonz! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 12, 2020

As a workaround, you could use this code for now:

let A = @myClassDecorator class {
  ...
@xaviergonz

This comment has been minimized.

Copy link
Author

@xaviergonz xaviergonz commented Feb 13, 2020

Thanks for the workaround tip. Unfortunately I'm using create react app (typescript template) which uses babel underneath to transpile and the workaround doesn't work too well when the class is a generic one

let A = @myClassDecorator class <T>{
  x!: T
}

const a: A<number> | undefined  // 'A' refers to a value, but is being used as a type here.
// or
type A = typeof A
const a: A<number> | undefined  // Type 'A' is not generic.

// vs

@myClassDecorator
class B<T> {
  x!: T
}

const b: B<number> | undefined  // ok

also this is not valid in TS

let A = @myClassDecorator class {}
@xaviergonz

This comment has been minimized.

Copy link
Author

@xaviergonz xaviergonz commented Feb 13, 2020

I think the workaround I'll use in the end is

@myClassDecorator
class A {
  foo() {
    return new _A(...);
  }
}
const _A = A

but still, it would be great if there was no workaround needed at all I think :)

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.